Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend assignment_operator to optionally allow '=' assignments #2698

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MichaelChirico
Copy link
Collaborator

Closes #2441. Tests are partially copied from #2521 -- for implementation I'm starting from scratch rather than trying to re-work it based on the discussion there. Thanks @J-Moravec for getting things started & the fruitful discussion!

Pausing here for, well, even more discussion :)

I'm getting hung up on the interaction of the proposed argument with the other arguments, particularly allow_right_assign and allow_pipe_assign.

If we have (allow_right_assign=FALSE, top_level_operator="any"), that does make x -> 2 a bit ambiguous, right?

I can think of two options:

  1. Subsume the other options into this new one by allowing top_level_operator = c("<-", "->", "%<>%") to be equivalent to the current (allow_right_assign=TRUE, allow_pipe_assign=TRUE). That looks pretty transparent to me, but presents some back-compatibility issues.
  2. Ditch any in favor of either, i.e. either <- or =.

WDYT @IndrajeetPatil @AshesITR?

@AshesITR
Copy link
Collaborator

AshesITR commented Dec 9, 2024

From an interface standpoint I think top_level_operator should be called assignment_operator and "any" shouldn't be an option.

Maybe it's worth exploring what = assignment style should look like for inner assignments and reduce flexibility a bit when it comes to them.

@IndrajeetPatil
Copy link
Collaborator

I agree that we shouldn't support "any" as an option. That is effectively turning off the linter, which they can already do via the config file.

@MichaelChirico
Copy link
Collaborator Author

That is effectively turning off the linter, which they can already do via the config file.

Well, not exactly. It would still retain the remaining options -- even if we subsume all of allow_cascading_assign, allow_right_assign, allow_pipe_assign into one operator= argument, there's still the allow_trailing= argument that's independent of the assignment operator.

But maybe operator=NULL is the better way to say "any operator".

I think top_level_operator should be called assignment_operator

I didn't like the redundancy of assignment_linter(assignment_operator=...).

Maybe it's worth exploring what = assignment style should look like for inner assignments and reduce flexibility a bit when it comes to them.

I really wanted to punt here to make progress on this linter in the next release & enable using = even at some basic level. The discussion to resolving that in #2521 stalled a few times over 10 months. Not requiring <- is relatively highly requested; beyond the existing 👍 there I know we'd use it in {data.table} and I think {mlr} would use it too.

Using anything but <- for implicit assignments is already pretty niche, and implicit assignments are rare to begin with. It probably makes most sense to add a similar operator= argument there with similar behavior, but for implicit assignments. Glancing at whether it's prudent to subsume implicit_assignment_linter() into assignment_linter(), I'd say no -- it's not immediately clear how we'd cleanly include the three existing arguments to implicit_assignment_linter() in the assignment_linter() API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend assignment_linter() to enforce '=' by default for assignment
3 participants