pixelzoom
commented
3 years ago Assigning to @ariel-phet (development manager) to prioritize and assign this issue.
Closed NickCrews closed 3 years ago
pixelzoom
commented
3 years ago Assigning to @ariel-phet (development manager) to prioritize and assign this issue.
NickCrews
commented
3 years ago Made an example PR request at https://github.com/phetsims/chipper/pull/994
zepumph
commented
3 years ago Are there other methods of automatic formatting that I am missing?
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.
pixelzoom
commented
3 years ago FYI... This has been discussed in https://github.com/phetsims/chipper/issues/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 ?
NickCrews
commented
3 years ago 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 https://github.com/phetsims/chipper/pull/994, though I think there are architecture issues there, such as jsbeautifier only being runnable from within Chipper.
NickCrews
commented
3 years ago Oops, just saw @pixelzoom 's comment above mine. Will read that now.
Denz1994
commented
3 years ago 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: @zepumph will spend some time getting Prettier to comply with our standards as much as possible. Prettier does come with options for formatting and after playing around with a few examples it would be worth pursuing. @zepumph will compare a non-Prettier file with a Prettier file report back to the larger dev team to sign off on using Prettier, after noting its limitations.
@samreid suggested taking a look at the currently opened Prettier issues to note any blaring issue we may come across.
zepumph
commented
3 years ago MK/DB/JO: We don't want our code style to be a barrier to 3rd party contributors.
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.
NickCrews
commented
3 years ago 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 https://github.com/phetsims/chipper/pull/994, check it out, I'm not sure if that's what you meant by 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.
zepumph
commented
3 years ago 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 ratio-and-proportion, and I found that things looked a bit better with this patch:
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 https://github.com/phetsims/phet-info/issues/150#issuecomment-732344358 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.
zepumph
commented
3 years ago 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 (grunt format-all yielded 1000 changed files)
zepumph
commented
3 years ago From https://github.com/phetsims/chipper/pull/994#issuecomment-748355504, 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.
zepumph
commented
3 years ago I just added in Prettier to compare, and here are the things that I notice to be different:
0 to all decimals ".4" => "0.4"if ( this.ratio.movingInDirection() ) { => if (this.ratio.movingInDirection()) { 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 grunt format on your favorite repo, using both parsers for experimentation. Right now it isn't totally clear if js-beautify or Prettier is the better way to go.
npm install in chippergrunt format (which uses js-beautify) grunt format --prettier (which uses prettier as the formatter) 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!
NickCrews
commented
3 years ago Thanks so much @zepumph for keeping this rolling! 🚀 Looks like some great tweaks to my initial prototype.
I don't really care as to the details of how they look, and y'all are the real ones who have to live with it, so I recognize your preferences here are definitely more important (I just wanted an easy way to conform), but here are a few thoughts:
js-beaufity: (quoting @zepumph's analysis above)
- Newline at the end of each file (I like that)
- No object literals defined on one line, even when a function parameter
- Spacing on the 2nd and more line when you break up statements. In the new version, it is always one level more of indentation, but in webstorm, it tries to align it with the context.
I like 1, really like 3, and am ambivalent about 2. Also, now that we are using the sortImports grunt task, some imports are getting moved around. This is nice, I just wanted to call out that this patch isn't actually adding this sorting, it's just better enforcing a rule that's already there.
import BooleanProperty from '../../../../axon/js/BooleanProperty.js';
- import NumberProperty from '../../../../axon/js/NumberProperty.js';
import createObservableArray from '../../../../axon/js/createObservableArray.js';
+ import NumberProperty from '../../../../axon/js/NumberProperty.js';
import Range from '../../../../dot/js/Range.js';These are definitely the most common changes, there might be some others too. I very occasionally did run into @zepumph's same issue
- I also noticed that in some files, the current formatting adds a newline at the top of the file above the copyright. That is annoying.
it's not always above the copyright, it's just the first line in the file sometimes. It's a bug with sortImports I think, if I remove that part of the format task then the blank line is not added. @zepumph can you confirm?
prettier: Regarding @zepumph's noted differences above:
- Adds a
0to all decimals ".4" => "0.4"- All spacing around if statements, arrays, function parameters, and their parens/brackets disappears:
if ( this.ratio.movingInDirection() ) {=>if (this.ratio.movingInDirection()) {- Generally, where we have newlines and wrapping for comma separated items as well as object literals in parameters looks really different:
Ambivalent about 1. There are some other trivial differences with constants for 1, such as 1E-3 -> 1e-3. I like 2. I don't quite follow 3, but it looks like prettier doesn't care about a 80 character line limit, and just let's lists/args continue on one line as long as they like. I don't really have a preference.
There are a couple other differences I noticed:
4.
if () {
...
}
else if () {
...
}changes to
if () {
...
} else if () {
...
}which I like.
If I had to choose, I would go for prettier:
If you want moar opinions, a next step would be to think about how you want to enforce this, like could you set up a github action that runs this for you on a PR or after every push?
Thanks so much everyone!
samreid
commented
3 years ago After testing prettier on circuit-construction-kit-common, I would like to change my opinion on this issue.
Readable code is essential to code maintainability. While some aspects of code formatting are indeed "bike shedding" and would be fine either way, there are enough cases where one format is objectively clearer than another--and better enough that it warrants flexibility.
For example, here is some code in CCK:
createVertexPairArray( position, length ) {
return [
this.createVertex( position.plusXY( -length / 2, 0 ) ),
this.createVertex( position.plusXY( length / 2, 0 ) )
];
}After prettier, things no longer line up and it is harder to see what is happening:
createVertexPairArray(position, length) {
return [this.createVertex(position.plusXY(-length / 2, 0)), this.createVertex(position.plusXY(length / 2, 0))];
}Here's another example:
// Take an initial current sign, but avoid measuring noise from the matrix solves in disconnected circuits
if ( this.isInitialCurrentNegative === null ) {
this.isInitialCurrentNegative = current < -epsilon ? true :
current > epsilon ? false :
null;
}after prettier:
// Take an initial current sign, but avoid measuring noise from the matrix solves in disconnected circuits
if (this.isInitialCurrentNegative === null) {
this.isInitialCurrentNegative = current < -epsilon ? true : current > epsilon ? false : null;
} else if (isReadyToClear) {Prettier has an unusual way of formatting ternary operators:
CCK original:
range: options.highResistance ? new Range( 100, 10000 ) :
options.realistic ? new Range( 0, 1E6 ) : // The non-ohmic bulb has its resistance computed in ModifiedNodalAnalysisAdapter.js
new Range( 0, 120 )After prettier:
range: options.highResistance
? new Range(100, 10000)
: options.realistic
? new Range(0, 1e6) // The non-ohmic bulb has its resistance computed in ModifiedNodalAnalysisAdapter.js
: new Range(0, 120)It should read like:
isItX => doSomethingAboutX:
isItY => doSomethingAboutY:
otherwiseDoSomethingAboutZCCK
const verticalAquaRadioButtonGroup = new VerticalAquaRadioButtonGroup( schematicTypeProperty, [
{ node: new Text( circuitConstructionKitCommonStrings.ieee, textOptions ), value: SchematicType.IEEE, tandemName: 'ieeeRadioButton' },
{ node: new Text( circuitConstructionKitCommonStrings.iec, textOptions ), value: SchematicType.IEC, tandemName: 'iecRadioButton' }
], {
tandem: tandem.createTandem( 'schematicTypeRadioButtonGroup' ),
radioButtonOptions: {
radius: 9
}
} );After prettier:
const verticalAquaRadioButtonGroup = new VerticalAquaRadioButtonGroup(
schematicTypeProperty,
[
{
node: new Text(circuitConstructionKitCommonStrings.ieee, textOptions),
value: SchematicType.IEEE,
tandemName: 'ieeeRadioButton'
},
{
node: new Text(circuitConstructionKitCommonStrings.iec, textOptions),
value: SchematicType.IEC,
tandemName: 'iecRadioButton'
}
],
{
tandem: tandem.createTandem('schematicTypeRadioButtonGroup'),
radioButtonOptions: {
radius: 9
}
}
);In the last part of https://github.com/phetsims/circuit-construction-kit-common/commit/c7f915470e35d8a07340613cfcd12d7ae9f466c6, I was glad to be able to put the first 2 parameters on the invocation line:
CCK:
createHighVoltageBatteryToolNode( count, tandem ) {
return this.createCircuitElementToolNode( batteryString, count,
( tandem, viewTypeProperty ) => new BatteryNode( null, null,
new Battery(
new Vertex( Vector2.ZERO ),
new Vertex( new Vector2( CCKCConstants.BATTERY_LENGTH, 0 ) ),
new Property( 0 ),
Battery.BatteryType.HIGH_VOLTAGE,
Tandem.OPTIONAL, {
voltage: 1000
}Prettier:
createHighVoltageBatteryToolNode(count, tandem) {
return this.createCircuitElementToolNode(
batteryString,
count,
(tandem, viewTypeProperty) =>
new BatteryNode(
null,
null,
new Battery(
new Vertex(Vector2.ZERO),
new Vertex(new Vector2(CCKCConstants.BATTERY_LENGTH, 0)),
new Property(0),
Battery.BatteryType.HIGH_VOLTAGE,
Tandem.OPTIONAL,
{
voltage: 1000
}
),We also agreed as a team that we prefer to have a blank line between an if statement and its comment:
// never slow down or run the current backwards
if ( sameDirectionAsCurrent ) {
// When we need to correct in the same direction as current flow, do it quickly.
const correctionStepSize = Math.abs( 5.5 / NUMBER_OF_EQUALIZE_STEPS * SPEED_SCALE * dt );prettier:
// never slow down or run the current backwards
if (sameDirectionAsCurrent) {
// When we need to correct in the same direction as current flow, do it quickly.
const correctionStepSize = Math.abs((5.5 / NUMBER_OF_EQUALIZE_STEPS) * SPEED_SCALE * dt);This is not an exhaustive list of all the problems in circuit-constrcution-kit-common, and I didn't search other repositories.
I feel that looking at these cases in isolation somewhat trivializes the matter, since you can take the time to look at each example in isolation to understand it and rationalize that it is OK. But we are designing for large-scale and repeated application of patterns, so we should endeavor to reduce cognitive load and make the code as readable and maintainable as possible.
Also, in my opinion, these formatting cases are not rare enough that we would want to use directives to temporarily disable the formatter around them.
This issue was opened because of the difficulty of non-WebStorm users being able to contribute code that matches the PhET style guidelines. We do not want code formatting to be a hurdle to contributors. But in my experience, when we have had code contributions, we thank the contributor, then run a formatter ourselves when we integrate the code into our codebase. I don't think we should reject any code for noncompliant formatting. We also have PhET developers that use Sublime and (at least one of them) said it is not a burden to manually follow the PhET style guidelines. I don't believe we have a sublime plugin.
I would like to hear more about how code formatting was burdensome to @NickCrews or other contributors, and discussing how we can alleviate those problems without making our production codebase harder to read and maintain.
zepumph
commented
3 years ago @samreid, there were two different formatting modules to try, but I only heard you compare "prettier", which is less flexible. Did you do any comparing of js beautify by using "grunt format" without the prettier option? In my mind js beautify was more likely to be accepted by this dev team. What grunt command did you use?
pixelzoom
commented
3 years ago Unfortunately, I don't have time to participate in this issue by comparing modules. But I thought I'd weigh in to say that I share the prettier concerns raised by @samreid in https://github.com/phetsims/phet-info/issues/150#issuecomment-752703293.
zepumph
commented
3 years ago Unfortunately, I don't have time to participate in this issue by comparing modules.
Ideally as many as possible would be able to go to a repo written in your own code style with no working copy changes, run grunt format, and see what you think. If no one has done this by dev meeting, then likely no progress will be made on this. I will again repeat that it isn't clear to me what formatting tool @samreid was using in https://github.com/phetsims/phet-info/issues/150#issuecomment-752703293. If it was Prettier, then I think it is still worth having a conversation about js-beautify which grunt format is powered on (as opposed to grunt format --prettier), because it is much more flexible, and I found it quite close to our code style.
pixelzoom
commented
3 years ago @zepumph said:
I will again repeat that it isn't clear to me what formatting tool @samreid was using in https://github.com/phetsims/phet-info/issues/150#issuecomment-752703293.
The first sentence of that comment says:
After testing prettier on circuit-construction-kit-common, I would like to change my opinion on this issue.
... and then he mentions "prettier" at least 7 more times. So I'm assuming that he was using prettier.
zepumph
commented
3 years ago If that is indeed the case, then I am very interested to hear what both of you think about the current js-beautify strategy, which can be used with grunt format. Prettier to me was always much more of a stretch to think that we could make it work on the project. That is why it is only an option to grunt format right now. It is very inflexible. If we were starting a js project from scratch, perhaps we could learn to love it, but I also totally agree with @samreid's sentiments in https://github.com/phetsims/phet-info/issues/150#issuecomment-752703293.
samreid
commented
3 years ago My remarks in https://github.com/phetsims/phet-info/issues/150#issuecomment-752703293 were about prettier. I decided to test it first since:
Beautify introduces fewer problems than prettier did, and the problems are less severe. Here are some I spotted in CCK:
CCK:
const prefix = this.coefficient === 1 ? '' :
this.coefficient === -1 ? '-' :
this.coefficient + '*';Beautify changes indenting for subsequent lines (in many places), so things do not line up:
const prefix = this.coefficient === 1 ? '' :
this.coefficient === -1 ? '-' :
this.coefficient + '*';However, where this lining up is important, we could add a newline, then beautify will keep things lined up like so:
const prefix =
this.coefficient === 1 ? '' :
this.coefficient === -1 ? '-' :
this.coefficient + '*';CCK also had code commented like so:
// The sampled points for the wire/filament curves
const LIFELIKE_SAMPLE_POINTS = [
new Vector2( 0.623, 2.063 ), // bottom center
new Vector2( 0.623, 1.014 * 0.75 ), // first curve
new Vector2( 0.314 * LEFT_CURVE_X_SCALE, 0.704 * TOP_Y_SCALE * 1.1 ), // left curve 1
new Vector2( 0.314 * LEFT_CURVE_X_SCALE, 0.639 * TOP_Y_SCALE ), // left curve 2
new Vector2( 0.394 * LEFT_CURVE_X_SCALE, 0.560 * TOP_Y_SCALE ), // left curve 3
new Vector2( 0.823 * RIGHT_CURVE_X_SCALE, 0.565 * TOP_Y_SCALE ), // top right 1
new Vector2( 0.888 * RIGHT_CURVE_X_SCALE, 0.600 * TOP_Y_SCALE ), // top right 2
new Vector2( 0.922 * RIGHT_CURVE_X_SCALE, 0.699 * TOP_Y_SCALE ), // top right 3
new Vector2( 0.927 * RIGHT_CURVE_X_SCALE, 1.474 ), // exit notch
new Vector2( 0.927 * 0.8 * 1.2, 1.474 ) // exit
];which was beautified like so:
// The sampled points for the wire/filament curves
const LIFELIKE_SAMPLE_POINTS = [
new Vector2( 0.623, 2.063 ), // bottom center
new Vector2( 0.623, 1.014 * 0.75 ), // first curve
new Vector2( 0.314 * LEFT_CURVE_X_SCALE, 0.704 * TOP_Y_SCALE * 1.1 ), // left curve 1
new Vector2( 0.314 * LEFT_CURVE_X_SCALE, 0.639 * TOP_Y_SCALE ), // left curve 2
new Vector2( 0.394 * LEFT_CURVE_X_SCALE, 0.560 * TOP_Y_SCALE ), // left curve 3
new Vector2( 0.823 * RIGHT_CURVE_X_SCALE, 0.565 * TOP_Y_SCALE ), // top right 1
new Vector2( 0.888 * RIGHT_CURVE_X_SCALE, 0.600 * TOP_Y_SCALE ), // top right 2
new Vector2( 0.922 * RIGHT_CURVE_X_SCALE, 0.699 * TOP_Y_SCALE ), // top right 3
new Vector2( 0.927 * RIGHT_CURVE_X_SCALE, 1.474 ), // exit notch
new Vector2( 0.927 * 0.8 * 1.2, 1.474 ) // exit
];CCK has this code:
const toggleNode = new ToggleNode( this.viewTypeProperty, [
{ value: CircuitElementViewType.LIFELIKE, node: lifelikeIcon },
{ value: CircuitElementViewType.SCHEMATIC, node: schematicIcon }
] );JSBeautify doesn't like object literals on one line:
const toggleNode = new ToggleNode( this.viewTypeProperty, [ {
value: CircuitElementViewType.LIFELIKE,
node: lifelikeIcon
},
{
value: CircuitElementViewType.SCHEMATIC,
node: schematicIcon
}
] );CCK:
lifelikeFuseNode.mutate( { scale: width / lifelikeFuseNode.width } );jsbeautify:
lifelikeFuseNode.mutate( {
scale: width / lifelikeFuseNode.width
} );The color table is much more difficult to parse:
const colorTable = [
{ name: 'none', significantFigure: null, multiplier: null, tolerance: 20, color: null },
{ name: 'pink', significantFigure: null, multiplier: -3, tolerance: null, color: new Color( 255, 105, 180 ) },
{ name: 'silver', significantFigure: null, multiplier: -2, tolerance: 10, color: new Color( 192, 192, 192 ) },
// chose a different color that shows up better against the resistor color
{ name: 'gold', significantFigure: null, multiplier: -1, tolerance: 5, color: new Color( '#e6b600' ) },
{ name: 'black', significantFigure: 0, multiplier: 0, tolerance: null, color: new Color( 0, 0, 0 ) },
{ name: 'brown', significantFigure: 1, multiplier: 1, tolerance: null, color: new Color( 150, 75, 0 ) },
{ name: 'red', significantFigure: 2, multiplier: 2, tolerance: null, color: new Color( 255, 0, 0 ) },
{ name: 'orange', significantFigure: 3, multiplier: 3, tolerance: null, color: new Color( 255, 165, 0 ) },
{ name: 'yellow', significantFigure: 4, multiplier: 4, tolerance: null, color: new Color( 255, 255, 0 ) },
{ name: 'green', significantFigure: 5, multiplier: 5, tolerance: null, color: new Color( 154, 205, 50 ) },
{ name: 'blue', significantFigure: 6, multiplier: 6, tolerance: null, color: new Color( 100, 149, 237 ) },
{ name: 'violet', significantFigure: 7, multiplier: 7, tolerance: null, color: new Color( 148, 0, 211 ) },
{ name: 'gray', significantFigure: 8, multiplier: 8, tolerance: null, color: new Color( 160, 160, 160 ) },
{ name: 'white', significantFigure: 9, multiplier: 9, tolerance: null, color: new Color( 255, 255, 255 ) }
];jsbeautify:
const colorTable = [ {
name: 'none',
significantFigure: null,
multiplier: null,
tolerance: 20,
color: null
},
{
name: 'pink',
significantFigure: null,
multiplier: -3,
tolerance: null,
color: new Color( 255, 105, 180 )
},
{
name: 'silver',
significantFigure: null,
multiplier: -2,
tolerance: 10,
color: new Color( 192, 192, 192 )
},
// chose a different color that shows up better against the resistor color
{
name: 'gold',
significantFigure: null,
multiplier: -1,
tolerance: 5,
color: new Color( '#e6b600' )
},
{
name: 'black',
significantFigure: 0,
multiplier: 0,
tolerance: null,
color: new Color( 0, 0, 0 )
},
{
name: 'brown',
significantFigure: 1,
multiplier: 1,
tolerance: null,
color: new Color( 150, 75, 0 )
},
{
name: 'red',
significantFigure: 2,
multiplier: 2,
tolerance: null,
color: new Color( 255, 0, 0 )
},
{
name: 'orange',
significantFigure: 3,
multiplier: 3,
tolerance: null,
color: new Color( 255, 165, 0 )
},
{
name: 'yellow',
significantFigure: 4,
multiplier: 4,
tolerance: null,
color: new Color( 255, 255, 0 )
},
{
name: 'green',
significantFigure: 5,
multiplier: 5,
tolerance: null,
color: new Color( 154, 205, 50 )
},
{
name: 'blue',
significantFigure: 6,
multiplier: 6,
tolerance: null,
color: new Color( 100, 149, 237 )
},
{
name: 'violet',
significantFigure: 7,
multiplier: 7,
tolerance: null,
color: new Color( 148, 0, 211 )
},
{
name: 'gray',
significantFigure: 8,
multiplier: 8,
tolerance: null,
color: new Color( 160, 160, 160 )
},
{
name: 'white',
significantFigure: 9,
multiplier: 9,
tolerance: null,
color: new Color( 255, 255, 255 )
}
];CCK:
parse: string =>
string === 'electrons' ? CurrentType.ELECTRONS :
string === 'conventional' ? CurrentType.CONVENTIONAL :
string // Will error out in validValues checkjsbeautify puts the ternary on one line:
parse: string =>
string === 'electrons' ? CurrentType.ELECTRONS : string === 'conventional' ? CurrentType.CONVENTIONAL : string // Will error out in validValues checkMost of the problems described above also appear in Fourier/jsbeautify.
How about a compromise:
(a) We acknowledge that we do not have good support for 3rd party contributors to match our code style. We do not expect contributors to be able to match our code style exactly, and we remain tolerant of non-formatted submissions. We can format it as desired when we bring it to our production code base.
(b) In order to provide partial support for contributors or developers not using WebStorm, we leave the jsbeautify grunt format as an option which can be used as desired.
UPDATE: Option (c) would be to advocate for contributors to use grunt format and provide them a short set of instructions how to manually align its output with our desired format. I think that would involve lining up multi-line expressions and removing a whitespace after function declarations. NOTE: I'm brainstorming this option for completeness, not recommending it above other options.
pixelzoom
commented
3 years ago IntelliJ IDEA can format code from the command line, see https://www.jetbrains.com/help/idea/command-line-formatter.html. Could this be done on the server as a nightly task?
NickCrews
commented
3 years ago I would like to hear more about how code formatting was burdensome to @NickCrews or other contributors, and discussing how we can alleviate those problems without making our production codebase harder to read and maintain.
I guess it comes down to how hard you want to follow the mantra no one should have to worry about formatting code. When I was working with @chrisklus, he had his formatter set up one way, I had mine another, and they constantly fought with each other. It was both time consuming and untenable for either of us to try to manually follow one of our styles, so we got a mix. I couldn't easily use hist formatter without switching up my entire IDE, disturbing my workflow. Sometimes formatting noise would leak into commits, making git history hard to understand. That formatting noise also would be an argument against a nightly cleanup of your codebase, and a reason why code cleanly formatted form the beginning is important. I guess if only 5% of your code is from 3rd parties and is going to be run cron cleaned up, then probably not a big deal. I personally would be very annoyed and not likely to contribute if I had to manually follow formatting guidelines as proposed in @samreid's option c) above. Either a) or b) would be fine for me as an external contributor, it's just a matter of how much y'all internally care. I guess in summary, if I were to contribute as an 3rd party, I do not want to have to worry about manually formatting my code. Either accept as is, or give me a command to run that isn't IDE-tied. Also that's just my opinion, perhaps solicit opinions from your previous 3rd party contributors?
Y'all have to look at this code way more than me, so again take this with a grain of salt, but I would look again at the issues that you're finding with prettier. React, jest, Gatsby, and 2 million other projects think prettier is good enough. I personally actually like prettier's look more in many of the examples above, and thought the others were a toss-up. Automatic and opinionated code formatting is a popular phenomena showing up everywhere, such as gofmt with Go, rustfmt with Rust, black with Python, etc, and I think there is something to that hype that should be considered. Once you make the transition, you will never have to worry about it again. Do you think you get used to the new style? I was once forced to switch to a style I wasn't used to and I hated it, but then gradually forgot about it. In the end though, me touting automatic and opinionated formatters is just my personal preference, so follow your preferences. The contrasting, valuable motto is "don't fix what ain't broken".
Actually, as I think about it more, I would only recommend using prettier, and not jsbeautify, because of prettier's popularity and guaranteed support. It's more different from your current style, but again I would think about the long-term cost, not the transition cost. Also I would +1 to @samreid's comment above that WebStorm has built in prettier support, that is another reason for it.
zepumph
commented
3 years ago Thanks! That is very helpful for me to hear.
My vote is 100% towards an automated solution, and I don't care at all about what the code looks like. I'll get used it. I hear the argument towards a project like Prettier which is better maintained and supported. I'm excited for a dev meeting discussion.
samreid
commented
3 years ago Thanks for reporting on your experience in collaborating with different code formatting tools--that's very helpful for us to better understand the problem we are facing. I agree we should work toward a fully automated and IDE-agnostic solution.
Regarding prettier: I think prettier has some suboptimal opinions that are not a good match for the legibility of our project. I pointed out patterns in https://github.com/phetsims/phet-info/issues/150#issuecomment-752703293 that are problematic, but I'm not the only one who feels prettier has suboptimal cases--Prettier has 658 open issues, and sorting by the most commented it is easy to find issues which basically say "prettier is making code objectively more difficult to read for case X", and some of these comments have hundreds of upvotes and no plans to be addressed. There is even a project from prettier called https://github.com/prettier/prettier-eslint which says:
One of the nice things about prettier is how opinionated it is. Unfortunately, it's not opinionated enough and/or some opinions differ from my own.
prettier-eslint runs prettier on the code, then runs ESLint with --fix and uses style rules to further format the code. But this leads to another solution that is worth investigation: We can use ESLint style rules with --fix to handle our formatting (without a prior formatting step). Here is an article about how that can be set up, and details about airbnb and google style configurations.
Here is a list of the ESLint style rules, many of which can be autofixed: https://eslint.org/docs/rules/#stylistic-issues
Using this, I was able to create a configuration that looked like a good fit. I tested prettier+eslint, and jsbeautify+eslint and eslint by itself. prettier+eslint still had some problems that I didn't see how to overcome, but jsbeautify+eslint seemed workable. But I don't even think jsbeautify will be necessary due to ESLint's coverage--eslint by itself may be best and I think we should investigate that first. To investigate further, I think we should take the following steps:
--fix that will run the autofix (there is commented out code in lint.js to do this now)--patterns that lets you specify one or more files to format. This will let us plug it in to our IDEs.Self-assigning to look into those steps. If upgrading ESLint from 6 to 7 is nontrivial I'll open a new issue for that.
For my reference, here is a first draft of rules I tested:
And a configuration I adapted from https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/rules/style.js
A few more revisions to the code style:
samreid
commented
3 years ago In the above issues, I updated to ESLint 7 and added options for --fix and --patterns. Here is a file of formatting rules that I adapted from the airbnb configuration. I relaxed several rules so they wouldn't conflict with master too much, but we could make this more strict as desired. (I tagged lines of code with TODO referencing this issue):
Basic testing instructions:
cd to a simulation directory and run grunt lint --fixWebStorm integration instructions:
grunt and Arguments lint --fix --patterns=$FilePath$Some additional pros and cons comparing eslint formatting vs prettier (in addition to pros and cons already mentioned previously), and some observations:
~/apache-document-root/main/fourier-making-waves$ time grunt lint --fix
Running "lint" task
linting ../fourier-making-wavesDone.
real 0m3.745s ~/apache-document-root/main/fourier-making-waves$ time grunt format --prettier Running "format" task
Done.
real 0m2.315s
* Adjusting rules with ESLint is straightforward: create code with the non-matching formatting and it will tell you what rule is in conflict:
One thing I like about the google configuration is that they list all of the rules, commenting out defaults, in order to make it easy to adapt as new rules are introduced: https://github.com/google/eslint-config-google/blob/master/index.js. If we go forward with ESLint formatting, maybe we should do the same.
I think more fine tuning should be done on the formatting, but I think this is at a good place to discuss.We discussed this today in dev meeting:
CM: WebStorm needs to know about code style for cursor placement, and live templates. Can we continue maintaining both the WebStorm XML in addition to the ESLint?
MK: ESLint --fix seems promising, how should we try it out?
CM: I'm not interested in seeing a lot of red underlined errors for formatting issues. SR: We could use "warn" for the style rules? MK: That seems good.
JO: I use sublime and I'm used to the proper formatting, and I have small tools that adjust formatting. I'd prefer not to have to switch from sublime to WebStorm to format code.
MK: Even though we mostly use WebStorm, I still run into formatting inconsistencies frequently.
CM: I'd like to understand the benefit of this non-trivial change. Our existing practice is streamlined. We would not reject any submissions that use a different format, we can simply format it upon receipt. It's not a big deal if some files are not formatted properly.
JB: It would be best if we could do this without negatively impacting our current workflow.
Discussion cut short, we should touch base on this next time.
zepumph
commented
3 years ago I would love to take a look at @samreid's lint rules and see if we can fine tune them to be more like the webstorm rules.
samreid
commented
3 years ago I tried running eslint without grunt, and it was much faster:
~/apache-document-root/main/fourier-making-waves$ time node ../chipper/node_modules/eslint/bin/eslint.js js --rulesdir ../chipper/eslint/rules/ --cache
real 0m0.308sThe same thing takes 3.746s with grunt. This is with the rule set checked in to master, not the patch from above. There may be a big difference in performance on Windows too.
zepumph
commented
3 years ago I'll add this back to dev meeting once I have a chance to look at it, I think this thursday.
NickCrews
commented
3 years ago ESlint looks like a promising direction! The work on ESLint looks great @samreid and @jonathanolson!
Regarding
JO: I use sublime and I'm used to the proper formatting, and I have small tools that adjust formatting. I'd prefer not to have to switch from sublime to WebStorm to format code.
There is this Sublime Plugin, seems like it has enough stars that maybe it's not trash?
If there is anything I could do in regards to this topic to be helpful, let me know. It is a different type of solution, but it was mentioned above: I have some experience using GitHub Actions as a CI/CD, to test or auto-format code before merge, would that be at all a useful direction?
zepumph
commented
3 years ago General thoughts I have about this issue:
grunt format task pretty much exclusively fix problems, and never error. I have some notes below about places where @samreid first pass may be at odds with that sentiment. I updated the CHIPPER/lint task to take a separate eslint rc file in addition to looking at our normal rules. This can be used with --format.
I have the following worries.
package.json in scenery to not error on bitwise opperatorsgrunt lint --fix --format could fix one of our non-format rules. Is that ok though?When I comment out the no-bitwise rule, things look really nice in general. Basically all the changes from our current code styles are some newlines here and there. That's awesome to see!
I got this error when running grunt lint --format, in joist.
Michael ~/PhET/git/number-line-common (master)
$ grunt lint-everything
Running "lint-everything" task
(Forwarding task to perennial)
Running "lint-everything" task
>> (node:27128) Warning: Accessing non-existent property 'padLevels' of module exports inside circular dependency
>> (Use `node --trace-warnings ...` to show where the warning was created)
TypeError: Cannot read property 'range' of null
Occurred while linting C:\Users\Michael\PhET\git\joist\js\simLauncher.js:9
at OffsetStorage.setDesiredOffset (C:\Users\Michael\PhET\git\chipper\node_modules\eslint\lib\rules\indent.js:341:45)
at C:\Users\Michael\PhET\git\chipper\node_modules\eslint\lib\rules\indent.js:1396:29
at Array.forEach (<anonymous>:null:null)
at Object.TemplateLiteral [as listener] (C:\Users\Michael\PhET\git\chipper\node_modules\eslint\lib\rules\indent.js:1388:34)
at C:\Users\Michael\PhET\git\chipper\node_modules\eslint\lib\rules\indent.js:1634:55
at Array.forEach (<anonymous>:null:null)
at Program:exit (C:\Users\Michael\PhET\git\chipper\node_modules\eslint\lib\rules\indent.js:1634:26)
at C:\Users\Michael\PhET\git\chipper\node_modules\eslint\lib\linter\safe-emitter.js:45:58
at Array.forEach (<anonymous>:null:null)
at Object.emit (C:\Users\Michael\PhET\git\chipper\node_modules\eslint\lib\linter\safe-emitter.js:45:38)
at NodeEventGenerator.applySelector (C:\Users\Michael\PhET\git\chipper\node_modules\eslint\lib\linter\node-event-generator.js:254:26)This is simLauncher line 9:
import asyncLoader from '../../phet-core/js/asyncLoader.js'; . . .
Thus --format doesn't work in joist currently.
In general I felt good enough to commit because most repos that I saw didn't have many changes from our webstorm formatting. Mainly I felt like using grunt lint --fix --format could unblock @NickCrews from any work he wanted to collaborate on. We can still fine tune process etc as we go, but from reviewing @samreid's rules, things feel pretty good to me. @samreid can you please review. From my understanding, the spark for this issue was not for @NickCrews to commit to phetsims repos, but just to collaborate more loosely. With that in mind I feel like the above commit unblocks him, and relieves the priority a bit.
Over to @NickCrews and @samreid to take it for a test drive and make recommendations..
samreid
commented
3 years ago Or I added it to the package.json in scenery to not error on bitwise opperators
Excellent, that sounds great.
If we want to prohibit [no-restricted-syntax], it feels like it doesn't belong in the formatting rules, but rather just in our general lint rules.
Let's move it to our general lint rules.
I don't like the idea that running grunt lint --fix --format could fix one of our non-format rules. Is that ok though?
Maybe we will conceptualize --format as that it adds the formatting rules rather than only using the formatting rules. If we conceptualize it like that, would we change the command line option name? Or how difficult would it be to change the implementation so that --format means to only use the formatting rules. I don't mind that it would auto-fix our other rules, developers should still be reviewing changes before a commit, so would hopefully notice things. Or even if they didn't notice, the auto-fix rules are very conservative and I wouldn't expect them to break anything.
linebreak-style unix made it so every newline I line gets changed. I have set up my git to save files in windows style, and commit in unix style. Do we need this lint rule?
This hasn't yet been a problem and I don't think we need to preemptively address it. If it becomes a problem in the future, we can look into it then.
Thus --format doesn't work in joist currently.
I also couldn't get it running in joist, but it wasn't because of that import line. I tried removing it and it failed similarly elsewhere.
I tested grunt lint --format --fix with some formatting changes in Fourier and didn't see any problems. I expected it to remove this whitespace but it didn't:
assert && assert( false, 'dispose is not supported, exists for the lifetime of the sim' );But I wouldn't consider that a blocking problem. Also, I saw 9 TODOs marked referencing this issue. I think it's at a reasonable state for now and I don't have plans to work on it in the near future. Since most of our team will continue to use WebStorm formatting, the priority seems somewhat reduced.
Assigning to @zepumph in case we would like to discuss any of the topics above. Leaving assigned to @NickCrews to take it for a test drive.
zepumph
commented
3 years ago Or how difficult would it be to change the implementation so that --format means to only use the formatting rules.
Ah ha! I found the ESLint option useEslintrc. Looks like setting that changes things to only lint from the rules we specify in lint.js. I don't think this is the way to go though, for example, in scenery it gets rid of the bitwise workaround we had, as well as causing problems like not recognizing the sceneryLog global (specified in its package.json). In addition I had to make a base type for the formatting and general rules to use, and I think that will be substantially harder to maintain.
Thus, here is the patch for a record, but I think we should keep things the way they are. After we --format --fix everything in our code base (which will basically just add newlines), it shouldn't be difficult to see how --fix may fix other lint rules and investigate.
My goal would be that there are no non-fixable lint rules in the formatting set, we found no-restricted-syntax in our previous conversation about, but I also noted new-cap as one that isn't really fixable, so I added it to the general rules in https://github.com/phetsims/chipper/issues/1010.
TODOS for this issue still:
--format --fix with some formatting changes in Fourier and didn't see any problems. I expected it to remove this whitespace but it didn't: assert && assert( false, 'dispose is not supported, exists for the lifetime of the sim' );zepumph
commented
3 years ago We could create a grunt task that just forwards to grunt lint --format --fix, but I think that will be more confusing than helpful.
zepumph
commented
3 years ago I tested grunt lint --format --fix with some formatting changes in Fourier and didn't see any problems. I expected it to remove this whitespace but it didn't: assert && assert( false, 'dispose is not supported, exists for the lifetime of the sim' );
I added no-multi-spaces above, and that fixes this. Presumably we can continue to hone in these rules as we use them more.
samreid
commented
3 years ago Upon reflection, I would like to recommend a change of course which I realized after working on https://github.com/phetsims/chipper/issues/1010. The proposal in the preceding comments is that we divide our rules into 2 categories:
(1) our main lint rules which don't address formatting and are not fixable (2) purely formatting rules which are all fixable
I don't think this is the right dimension in which to draw the line. Why should we disallow formatting rules from our main rules? Why shouldn't our main rules be fixable? I think it will be better for our project if we instead divide it like so:
(1) our main lint rules which we have agreed upon as a team and which all repos satisfy. These could include formatting or fixable rules. (2) supplemental rules that do anything we cannot do in (1). For now, this would include fixable formatting rules, but could be generalized to do other things. I think we should attempt to minimize (2) as much as possible to keep things simple.
For example, presume for the sake of discussion that our code review style says we want to have spaces in parens always (maybe it does). So why not put 'space-in-parens': [ 'error', 'always' ], in our primary rules? I recalled at one meeting, there was an objection that we don't want to see formatting errors while writing code (before we press auto-format), but I would like to question that. For instance, if I forgot a space, why wouldn't I want to be aware of that? It doesn't slow down the development experience, it seems non-intrusive. It is easy to press autoformat and see the problem disappear.
On the other hand, I haven't tested whether this slows down linting significantly--I would prefer to avoid death by a thousand cuts. But if we draw the line by speed, then we would split into (1) fast/important rules and (2) slow/secondary rules.
zepumph
commented
3 years ago I totally understand your argument to inline as many rules as possible in our general rules. I feel like I am less confident doing that now because I don't understand our formatting rules very well. In an ideal world, webstorm would perfectly work with the --format --fix rules, and would get rid of any underlining of errors from formatting. I feel like we will learn more as we use --format more. I would like to eventually format/fix our whole code base, but I want to take the necessary steps to get there first. Anything that changes between master and calling format --fix is totally fluff, and doesn't matter to our project's code quality. That said it would still error out, and not be fixable by webstorm. So by that argument there MUST be a second file for rules like that, or we must expect all devs to adopt grunt as our formatting tool. Over time we will understand these rules more, and could make a more informed decision.
This is not a topic that I can continue to spend this much time on (even though I want to). Thus my goal is to sprinkle this in to my work flow, and hopefully get our daily grunt task using --fix every night, to keep our codebase consistent. There are many improvements that can be made here without harming dev workflows in jetbrains.
samreid
commented
3 years ago Thus my goal is to sprinkle this in to my work flow, and hopefully get our daily grunt task using --fix every night, to keep our codebase consistent.
I thought during dev meeting, we decided against an auto-commit that changes formatting every night?
zepumph
commented
3 years ago I thought during dev meeting, we decided against an auto-commit that changes formatting every night?
I thought only to the extend that webstorm formatting would fight the auto formatting. Even if it is just space difference. I wouldn't want an automatic process fighting dev's formatting. First I want to make sure that webstorm and --format linting are completely compatible.
samreid
commented
3 years ago In https://github.com/phetsims/phet-info/issues/155 I formatted everything and showed a scope that would make it easy to reformat js code in the future. Now that that's done, it will be easier to check which of the proposed formatting rules are compatible with our WebStorm rules. If a formatting rule is 100% compatible with our WebStorm rules, is there a good reason not to move it to our main lint rules? Moving formatting rules to our standard lint will make it more likely that we commit formatting-compatible code and find fewer discrepancies when we run processes like https://github.com/phetsims/phet-info/issues/155 in the future.
For instance, I moved 'array-bracket-spacing': [ 'error', 'always' ] from the format rules to the main rules, and confimred that grunt lint-everything --disable-eslint-cache still passes. I timed with and without this rule (for linting everything, and the difference was in the noise):
Without array bracket spacing: 0m54.438s With array bracket spacing: 0m55.058s
This seems advantageous, because now (a) continuous testing (b) our precommit hooks (c) build steps and (d) IDEs will all catch this problem before it becomes a problem. I'll commit this since it seems like a step in the right direction.
samreid
commented
3 years ago First I moved the array-bracket-spacing rule to the main rules (as described in https://github.com/phetsims/phet-info/issues/150#issuecomment-786695043) , and it seemed so nice that I moved other rules that were passing or almost passing. time grunt lint-everything --disable-eslint-cache takes 1m4.294s but in my opinion that sounds worth it. Several other rules remain in the format eslint file.
Aside from checking in with @NickCrews, there are some remaining todos mentioned in https://github.com/phetsims/phet-info/issues/150#issuecomment-785306768. I'll copy them here for convenience:
--format --fix with some formatting changes in Fourier and didn't see any problems. I expected it to remove this whitespace but it didn't: assert && assert( /* several whitespaces here instead of this comment */ false, 'dispose is not supported, exists for the lifetime of the sim' );EDIT FROM MK:
NickCrews
commented
3 years ago Hey, I'll try to check this out within the next couple days, thanks!
samreid
commented
3 years ago The rule failing in joist is indent in simlauncher.js. Commenting out import() gets it working again. ESLint recently approved support for import() in https://github.com/eslint/eslint/issues/11486 but perhaps our version of ESLint is too old.
I tried updating all of the eslint versions in chipper (including the babel part), but it still had the same problem.
samreid
commented
3 years ago I opted out of the indent rule for simLauncher.js and now you can format joist (aside from indenting that file).
zepumph
commented
3 years ago https://github.com/davedoesdev/webauthn-perk/commit/f9be9c7eb6a8b2bddf72e9d48838523476cce8fe makes it seem like we may already have the right version, but perhaps need to specify the right parser.
zepumph
commented
3 years ago This did not work with the indent rule:
Index: eslint/format_eslintrc.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/format_eslintrc.js b/eslint/format_eslintrc.js
--- a/eslint/format_eslintrc.js (revision 7fe51ae0d781eb7aad2280a7bd3aa2f635026056)
+++ b/eslint/format_eslintrc.js (date 1614364500173)
@@ -400,6 +400,7 @@
browser: true,
es6: true
},
+ 'parser': 'babel-eslint',
parserOptions: {
ecmaVersion: 8,
sourceType: 'module'Your commit looks just fine.
zepumph
commented
3 years ago I'll commit to joist with format --fix.
samreid
commented
3 years ago I'll commit to joist with format --fix.
That is not currently compatible with the WebStorm formatter.
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?