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

Line length based code formatting #54

Open
gingermusketeer opened this issue Nov 4, 2017 · 10 comments
Open

Line length based code formatting #54

gingermusketeer opened this issue Nov 4, 2017 · 10 comments

Comments

@gingermusketeer
Copy link
Member

Now that we have cleaned up the config significantly as part of #2 it might be a good time to start thinking about how we go about implementing line length based code formatting.

Both @danielma and I have had a go at implementing this. We both used the printing algorithm described by Philip Wadler in his paper "A prettier printer" which can be found here.

We have taken different approaches for the implementation. You can see the WIP code for @danielma's here and my WIP code here.

This is the same algorithm used by Prettier. They have some descriptions of the commands used to power the formatter here.

The way that Prettier has been implemented can best be described by this flow:
Source -> Doc -> Formatted Source. Where Doc is an intermediary representation of the source code. It is this intermediary representation that is then printed with the line length aware formatter.

This approach has the advantage of the Doc construction phase being the only language aware component. This allows for the same document construction utilities and formatter to be used for different languages. For example:

  • Ruby Source -> Doc -> Formatted Source
  • CSS/SCSS Source -> Doc -> Formatted Source
  • HTML ERB Source -> Doc -> Formatted Source

The downside to this approach is likely performance. I don't have any numbers to back this up but if we are doing more work then we are currently I would hazard to guess that the performance will decrease to some degree.

So I think we have a few decisions to make:

  1. Should we use the algorithm used by Prettier?
  2. Should we implement in a decoupled manner such that we can format other languages?
  3. Are we okay with sacrificing some performance to gain line length aware formatting?
  4. OO (@danielma's approach) or mostly Functional (my approach)?
  5. How should we approach implementation?
@gingermusketeer
Copy link
Member Author

gingermusketeer commented Nov 4, 2017

Here are my answers:

  1. Yes 😄
  2. Yes
  3. Yes - As long as it is still usable for the first version and tuning is prioritised subsequently to improve performance.
  4. Mostly Functional.
  5. Collaborate on a branch on this repo to implement line length aware formatting for a subset of ruby code handled by the existing formatter.
    • This allows the approach to be used and potentially released from master ASAP.
    • Avoids long living branches that are given up on.
    • We may be able to divide and conquer on other parts of the language once the first PR is merged.
    • I suggest that we pick off hashes, arrays or parameters for this initial effort.

@danielma
Copy link
Member

danielma commented Nov 5, 2017

👏 thanks for writing this up!

First, my simple answers

\1. Yes
\3. Yes
\4. Whichever works better 🙂. I think you've done a better job of implementing the prettier paper than I did.

Second, my longer form answers

For 2, I would say no. I think it's a nice idea in theory, but I don't think the extra effort is worth it. Ruby isn't a fast language, and heavily version dependent. A generic tool for parsing code and pretty-printing is better suited to a faster language that supports compilation so it is easily portable.

For 5, I think you've laid out something nice. I also started from a similar place, but ran into roadblocks.

implement line length aware formatting for a subset of ruby code handled by the existing formatter.

I think this is a great idea. If we can get your WIP code to pass specs, I think it will be awesome, and the path you've laid out is really nice. The reason I worked on a rewrite is because I felt that rufo's original formatter wasn't set up well to work with an intermediate representation, and found myself getting into rabbit hole after rabbit hole of changes that had to be made to keep specs passing.

Avoids long living branches that are given up on.

You're probably referring to me 😆 . I know my branch looks like it's been given up on, but that's not the full story. My company gives me 2 weeks every 6 months to work on any project I choose, so I spent the last chunk of time on rufo. I intend to spend my next chance (in December) to spend those 2 weeks also working on rufo. So my branch hasn't been given up on, but I just haven't had time to invest since August.

So, I'll have a big chunk of time to give in mid December. I was planning to work on my fork, but if we come up with a better plan, I will work on that

I suggest that we pick off hashes, arrays or parameters for this initial effort.

This is a great place to start. If we can prove out the concept here, we're on a great path.

@gingermusketeer
Copy link
Member Author

@danielma Thanks for the great input.

I didn't have your fork in mind particularly when talking about "Avoids long living branches that are given up on." was reflecting on my own changes in motivation for projects as time goes on.

RE your work to rewrite rufo. I wonder if I am being naive in that approach based on what you said above. That said I think it is still worth trying.

That is a very cool initiative, sounds like you work for a great company :)

I will look to make a start on this and will keep you updated as I progress. If anyone wants to help out with the initial changes sing out and I can push up a WIP branch to collaborate on.

@bessey
Copy link
Member

bessey commented Nov 9, 2017

I want to help! I don't have such company benefits, but I do have evenings and weekends :)

  1. Definitely, I have no idea how to proceed if we don't follow this plan :p
  2. I don't think so. We don't commonly write CSS / JS / GraphQL in Ruby. Actually, I totally do write GraphQL in Ruby a lot haha, but anyway, I don't think the potential overhead this may bring is worth it for now. We can of course keep this in the back of our minds, and perhaps where there is low overhead, do so.
  3. Yes, though perhaps worth creating a bench suite so we know what kind of tradeoff we're making.
  4. No comment, haven't had a chance to compare them yet. I imagine implementing as close to the paper initially is more likely to see success, but not in a position to comment. Perhaps refactor to OO once we have a working version.
  5. Whatever you want, I'll follow. I like your ideas above.

@mjago
Copy link
Member

mjago commented Nov 10, 2017

I've still to get my head around the prettier printer so I can't comment on strategy, but I'll help where I can.
In terms of performance could line length awareness be configurable?

@flyerhzm
Copy link
Contributor

@gingermusketeer what's the status? any reason the PR is not merged into master branch?

@gingermusketeer
Copy link
Member Author

@flyerhzm You can see the work in progress here #91

The main issue that is holding it up is that heredocs are problematic to get formatted right (or at least in a way that is valid ruby). Only a few tests failing but until they have been resolved we can't merge that branch.

@gingermusketeer
Copy link
Member Author

If you have any suggestions to resolve the tests that are still failing let me know!

@gingermusketeer
Copy link
Member Author

gingermusketeer commented Feb 23, 2019

Made some good progress on this front today. All the rspec tests are passing now on 2.6.1 :)

@bmulholland
Copy link

Looks like #91 was merged a year ago - is there anything else holding up length-based formatting now?

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

No branches or pull requests

6 participants