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

Automatic LF code formatter #1227

Merged
merged 138 commits into from
Sep 2, 2022
Merged

Automatic LF code formatter #1227

merged 138 commits into from
Sep 2, 2022

Conversation

petervdonovan
Copy link
Collaborator

@petervdonovan petervdonovan commented Jun 13, 2022

Tasks:

  • Implement missing cases in visitor pattern
  • Implement line wrapping (Billy, Peter) -- (edit: @petervdonovan is making another pass at this)
  • Preserve comments - @petervdonovan
  • Allow the user to disable formatting for specific sections of their code (see comments below) - @petervdonovan
  • Add CI check that applies formatter to all *.lf files in the test directory and checks for equivalence (@lhstrh)
  • Add methods for testing semantic equivalence of AST subtrees
  • Rename org.lflang.lfc to org.lflang.cli - @billy-bao
  • Create lff package and create lff application that applies formatter - @billy-bao
  • Factor out WIP toward new FedGenerator (to be submitted in a separate PR)

billy-bao and others added 2 commits June 13, 2022 21:59
Co-authored-by: Marten Lohstroh <[email protected]>
String-based line-wrapping for formatter
@billy-bao billy-bao assigned billy-bao and unassigned billy-bao Jun 14, 2022
@petervdonovan
Copy link
Collaborator Author

Can we try to merge this soon?

I promise that the change I just made was trivial and that this has mostly just been awaiting review for 23 days.

@lhstrh
Copy link
Member

lhstrh commented Aug 30, 2022

Sorry, this was not on my radar as my review wasn't requested. I'll have a look at this soon. I agree, we should get this merged...

@lhstrh lhstrh self-requested a review August 31, 2022 20:13
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

This looks great and almost ready to merge! 🚀 🚀 🚀 I left a few comments. The formatting of the tests looks excellent to me, modulo a single bike shedding issue: semicolons. I believe it makes sense to enforce them in the LF code if the target requires them, and I recall @edwardalee expressing the same preference.

@lhstrh lhstrh changed the title Add capability to generate LF code and create a CLI program for formatting Automatic LF code formatter Aug 31, 2022
@lhstrh lhstrh added the feature New feature label Aug 31, 2022
@lhstrh lhstrh mentioned this pull request Sep 2, 2022
@lhstrh lhstrh merged commit f71bc7e into master Sep 2, 2022
@lhstrh lhstrh deleted the pretty-printer branch September 2, 2022 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants