Skip to content

Minor refactor of better for desugar #23448

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

Closed
wants to merge 2 commits into from

Conversation

som-snytt
Copy link
Contributor

Move the complex guards into the body of the case and use an if conditional to handle is better fors or legacy.

The list of enumerators is split into a prefix of aliases and suffix of remaining. The guards made this obscure. (Probably the prefix of aliases is not long, so taking the span is cheap enough.)

Make the duplicated code in the last case a more exact copy. (But don't bother to DRY it out.)

@som-snytt som-snytt force-pushed the refactor/desugar-for branch from f2a2d13 to 4460166 Compare July 11, 2025 23:52
@som-snytt som-snytt marked this pull request as ready for review July 11, 2025 23:52
@Gedochao Gedochao requested a review from KacperFKorban July 14, 2025 09:28
Copy link
Member

@KacperFKorban KacperFKorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, but I'm hesitant to merge changes that are primarily stylistic.
Readability is very subjective, and every time someone does a style refactor, other people have to re-learn the code.

@som-snytt
Copy link
Contributor Author

In this case, I think the guard conditions are objectively hard to read. (As I learned when I had to read them.)

The guard condition says dropWhile and takeWhile which is probably less inefficient than the use of span in the commit.

The existing guard might be more conservative when the new feature is experimental (and the rest of the condition is never evaluated), but presumably the feature is actually exercised now.

I think the readability argument carries more weight, but if you don't see the value, then feel free to ignore the PR.

@som-snytt
Copy link
Contributor Author

I wished to say, if you feel that the potential benefit is outweighed by the work to review the change, please ignore the PR.

Copy link
Member

@KacperFKorban KacperFKorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sure I can be convinced that this improves readability and doesn't change it that much from the spec.

My main concerns were just that (1) the structure of the code looks less like the structure of the spec now (2) I want to see myself (and other people that already know the code) time from re-learning the same code, just to do a small change/review.

Sorry for the long reply times.

@som-snytt
Copy link
Contributor Author

I'll look at whether it reflects the spec. It's certainly an Oderskyism that code should structurally preserve its specification (looking at parser, typer).

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.

2 participants