-
Notifications
You must be signed in to change notification settings - Fork 27
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
Improve Automatic Code Formatting #150
Comments
Assigning to @ariel-phet (development manager) to prioritize and assign this issue. |
Made an example PR request at phetsims/chipper#994 |
I would say that the primary formatting ground-truth is the IDEA xml: https://github.com/phetsims/phet-info/blob/master/ide/idea/phet-idea-codestyle.xml. This is super interesting! We recently added pre-commit hooks for linting and unit tests, and I feel like this could be a huge benefit as well. Unfortunately the idea xml is really the only maintained code style. I'm not sure about https://github.com/phetsims/phet-info/blob/master/ide/sublime-text/.jsbeautifyrc. We don't maintain that, since only one dev at PhET uses sublime. I don't know what it would take to convert the XML idea codestyle to js-beautify and to maintain it. I agree that automation would be pretty nice here. I think it would be best to mark for dev meeting and see if people like the idea of trying to automate this. |
FYI... This has been discussed in phetsims/chipper#761 ("Change to a cross-platform enforceable code formatting style"). There has been no progress on that issue since July 2019. Perhaps we should close this issue as "duplicate", and continue the discussion in phetsims/chipper#761 ? |
Thanks @zepumph! I had a couple ideas of what questions would probably be useful to talk about in your meeting:
I would be willing to take lead on this, but I'm sure I would need a bit of help in there. As a first go I made a draft at phetsims/chipper#994, though I think there are architecture issues there, such as jsbeautifier only being runnable from within Chipper. |
Oops, just saw @pixelzoom 's comment above mine. Will read that now. |
From dev meeting on 11/23/20: JO/MK: Using js.beautify to match our pattern would be a step in the right direction. SR: I'm recalling that using Prettier did not work well with our typical code formatting. JO: I'm open to changing our styling format in comparison to Prettier. It would depend on what is easier to read and review some examples. We could also write our own formatter that fits our pattern. JG: There were also options in js.beautify that didn't line up with our code styles as well. MK/DB/JO: We don't want our code style to be a barrier to 3rd party contributors. JB/JO: I'm fine with making our code pattern more accessible to others at the cost of getting used to a new pattern. SR: After reading into Prettier more the options look capable enough that it would be worth investigating. Proposal: @samreid suggested taking a look at the currently opened Prettier issues to note any blaring issue we may come across. |
I want to make sure to highlight this point. It was the general consensus from today's meeting that though we have "preferences" for code formatting, we all seemed to feel that those should come second to being an inclusive, open-source project in which formatting/code-style is not a barrier to contribution. Great summary, thanks @Denz1994! My hope for a demonstration is to create a grunt task that devs can run on a repo and look at their working copy changes for how different the node auto formatter is from webstorm. This approach would be independent of implementation (prettier/jsbeautify), though from today's conversation I will be starting with Prettier. |
Thanks for looking at this more! Let me know if you want my help, I could do maybe 2hrs/week. I just updated my experimental phetsims/chipper#994, check it out, I'm not sure if that's what you meant by |
Wow. Sorry that 3 weeks have passed (rude). After taking a look, I felt like it was safe to merge to master. I npm installed and formatted master of Index: js/grunt/format.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/grunt/format.js b/js/grunt/format.js
--- a/js/grunt/format.js (revision 117f08eef680489922417e1bd5bc9de189097752)
+++ b/js/grunt/format.js (date 1608256956536)
@@ -40,7 +40,7 @@
},
"js": {
"allowed_file_extensions": [ "js", "json", "jshintrc", "jsbeautifyrc", "sublime-settings" ],
- "brace_style": "collapse-preserve-inline",
+ "brace_style": "end-expand",
"break_chained_methods": false,
"e4x": false,
"end_with_newline": true,
@@ -48,10 +48,10 @@
"indent_level": 0,
"indent_size": 2,
"indent_with_tabs": false,
- "jslint_happy": false,
+ "jslint_happy": true,
"keep_array_indentation": false,
"keep_function_indentation": false,
- "max_preserve_newlines": 0,
+ "max_preserve_newlines": 2,
"preserve_newlines": true,
"space_after_anon_function": false,
"space_before_conditional": true,
I feel like this is a nice example of what jsbeautify could do, but I also think it would be worth following advice in #150 (comment) to try to see if Prettier (more stars on github) could be acceptable for us. I think it may be best to set up a way to compare both. I'll add a temporary option for it. |
I see three consistent differences from the webstorm:
I would like to see what Prettier does side by side. You can test this now on master with grunt format, and grunt format-all. This is still quite experimental ( |
From phetsims/chipper#994 (comment), while we are experimenting, it would be best to not add this as a dependency to all of chipper's gruntfile, so I am going to lazily require it for now.
|
I just added in Prettier to compare, and here are the things that I notice to be different:
this.ratioFitnessProperty = new DerivedProperty(
[this.unclampedFitnessProperty],
unclampedFitness => Utils.clamp(unclampedFitness, this.fitnessRange.min, this.fitnessRange.max),
{
isValidValue: value => this.fitnessRange.contains(value)
}
); |
I think I am ready to get some feedback from other developers. For next dev meeting, please try out
In general they are both different, but I think that minor/medium alterations to our code style in exchange for automation is well worth it. Thanks @NickCrews for getting the ball rolling on this! |
Only format the results if we actually are going to print them
Only format the results if we actually are going to print them
…s/phet-info#150 The old number method is way harder to interpret, for instance with the `indent` rule does `2` mean two spaces, or level `error`?
…s/phet-info#150 ESlint has deprecated `false`, we should use 'readonly' instead. See https://eslint.org/docs/user-guide/configuring/language-options#using-configuration-files-1 This probably should get changed in all of PhET's .eslintrc files in all repos
Hey there! I'm an independent dev, working with my buddy @chrisklus on a personal project where we're using a lot of the PHET sim framework. It's been super fun so far to use, awesome work!
One pain point that I'm finding is code formatting. I want to follow the PHET style guide. As I understand it at this point you have plugins available for two IDEs, IDEA and Sublime. Are there other methods of automatic formatting that I am missing? Right now I managed to tweak the options of the beautifier plugin of my IDE Atom, but I can't get it to match your formatting style.
Assuming that those are the two options, I find a couple problems with them:
These make it hard for me to follow your guidelines, and I bet it's a hurdle for anyone who wants to contribute to PHET.
I'm no expert here, but I bet there are some pretty awesome tools now that could solve these problems. I would be willing to take a look into this depending on your reception.
My first thought would be:
Using the popular Node.js package js-beautify, wrapped in the Grunt package. As I understand it, PHET uses Node and Grunt extensively, so adding this dependency would be not bad. js-beautify uses the same .jsbeautifyrc config file that already is used in the Sublime package, so we should get the same functionality that some devs are using. As I understand it then it would be not too hard to wire up running the grunt wrapper whenever you want to, such as in a pre-commit hook, and IDE tool, or just from the command line whenever you want. I have not tested this though, it's all wild idle speculation, because I am a N00b as to how to add the tools to Chipper.
Any thoughts welcome. Am I looking for a problem to solve and I should just suck it up?
The text was updated successfully, but these errors were encountered: