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

Sort require statements #595

Open
samreid opened this issue Jul 26, 2017 · 10 comments
Open

Sort require statements #595

samreid opened this issue Jul 26, 2017 · 10 comments

Comments

@samreid
Copy link
Member

samreid commented Jul 26, 2017

@jonathanolson mentioned that he and some other developers sometimes like to alphabetically sort the require statements. I asked on skype and heard 3 "yesses" right away, this is in addition to @jonathanolson. Sublime had built in support for this. IDEA doesn't have built in support for it (though maybe a plugin), but it would be nice if we had a tool for this.

@samreid
Copy link
Member Author

samreid commented Jul 26, 2017

I added a grunt task that sorts require statements alphabetically. It seems like it is working OK on CCK code. But it could use broader testing and review.

@samreid
Copy link
Member Author

samreid commented Jul 26, 2017

Here is the grunt doc and usage:

          sort-require-statements  Sort the require statements for all *.js    
                                   files in the js/ directory. This assumes the
                                   code is formatted  with IDEA code style and 
                                   that require statements take one line each  
                                   (not split across lines).  The files are    
                                   overwritten.                              

samreid added a commit that referenced this issue Jul 26, 2017
samreid added a commit to phetsims/circuit-construction-kit-dc that referenced this issue Jul 26, 2017
samreid added a commit to phetsims/circuit-construction-kit-dc-virtual-lab that referenced this issue Jul 26, 2017
samreid added a commit to phetsims/circuit-construction-kit-common that referenced this issue Jul 26, 2017
@samreid
Copy link
Member Author

samreid commented Jul 26, 2017

This code would benefit from a regular expression.

@samreid
Copy link
Member Author

samreid commented Jul 26, 2017

Also, we could update this to sort by package if we want.

@samreid
Copy link
Member Author

samreid commented Jul 26, 2017

I'm done working on this, no plans to update further at the moment.

@samreid samreid removed their assignment Jul 26, 2017
@samreid
Copy link
Member Author

samreid commented Jul 26, 2017

Upon reflection, I thought it might be more useful to sort by repo then class name (so all DOT things are together, SCENERY things are together, etc.) . However, this would be incompatible with other team members using alphabetical sort (like in Sublime).

Note: this will also put model things together and view things together.

samreid added a commit to phetsims/circuit-construction-kit-common that referenced this issue Jul 26, 2017
@jonathanolson
Copy link
Contributor

Upon reflection, I thought it might be more useful to sort by repo then class name (so all DOT things are together, SCENERY things are together, etc.) . However, this would be incompatible with other team members using alphabetical sort (like in Sublime).

  • Pros: Easier for me to duplicate a line and change the name (for something in the same folder), things are more conceptually grouped (if checking for how many things are imported from JOIST, etc.). More Java-like, felt like the default before sorting lines was easier.
  • Cons: Harder to at-a-glance check if something is imported, harder to manually pick the place things should go (would need to rely on tooling for that more).

I'll be fine with either approach, but would only prefer path-sorting if sorting is an easy in-IDE shortcut in IDEA and Sublime.

@samreid
Copy link
Member Author

samreid commented Jul 27, 2017

It would also be possible to add a script or step that removes unused require statements. We would adapt or factor out lint.js to work on a single file and return a report. It would take 1-2 hours to build this, document it and integrate it with existing tools.

@pixelzoom
Copy link
Contributor

After Slack discussion in dev-public channel, we decided to sort by filename instead of path. @samreid has made it so in the above commit.

@samreid
Copy link
Member Author

samreid commented Sep 11, 2017

We also decided to run grunt sort-require-statements on every file now instead of letting those kind of changes sneak in incrementally.

samreid added a commit that referenced this issue Sep 11, 2017
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

3 participants