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

Don't rewrite ~ @foo to ~@foo #345

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented Aug 12, 2024

The problem is ~ @foo is being reformatted to ~@foo, a different expression. ~,@foo is used as the canonical form because it seems to have more emphasis and stability than ~ @foo (just by my taste, lmk).

This decision has knock-on effects if there are are other forms between ~ and @ which are demonstrated in the tests.

The implementation uses transform twice, but it should ideally just be one tree pass. Please let me know if there's a preferred way to do this.

@frenchy64 frenchy64 force-pushed the ws-resolved-ambiguity branch 2 times, most recently from 093930a to d2522f3 Compare August 12, 2024 22:59
@frenchy64 frenchy64 marked this pull request as ready for review August 12, 2024 23:00
@weavejester
Copy link
Owner

Thanks for the bug report, and the PR. Can you explain why you used a volatile? I don't understand why it's necessary. Also, why is ~ @foo rewritten to ~,@foo? Why not keep it as ~ @foo?

@frenchy64
Copy link
Contributor Author

Can you explain why you used a volatile? I don't understand why it's necessary.

Starting with the premise of using transform twice, I used a volatile as a performance optimization to only perform the second walk if absolutely necessary.

It would be better to have just one walk but I didn't know the best way to do this.

Also, why is ~ @foo rewritten to ~,@foo? Why not keep it as ~ @foo?

This part is up to you.

My thought was: it's a really strange example of nested syntax that requires a space between the container and the child. I can't think of any other examples like it.

My idea with ~,@foo was to make the disambiguation more explicit to draw attention to itself.

@weavejester
Copy link
Owner

I think I'd like to know why cljfmt is removing the space in the first place. In the PR, code is changed for insert-missing-whitespace, presumably to re-insert the space that's removed by some other mechanism, but why not fix the code that causes it to remove the space in the first place?

@frenchy64
Copy link
Contributor Author

Ah, I misinterpreted insert-missing-whitespace as reinserting essential whitespace that was removed by cljfmt. I'll look into your suggestion.

@frenchy64
Copy link
Contributor Author

@weavejester seems to work!

@weavejester
Copy link
Owner

Much better! I don't think I have any more notes. Could you squash your commits down?

The problem is ~ @foo is being reformatted to ~@foo, a different
expression. We fix this by not treating spaces between ~ and @
as "surrounding spaces".
@frenchy64 frenchy64 force-pushed the ws-resolved-ambiguity branch from 1f8cda5 to 7734fe2 Compare August 13, 2024 00:17
@frenchy64
Copy link
Contributor Author

Done.

@weavejester weavejester merged commit fe8ae0e into weavejester:master Aug 22, 2024
1 check passed
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