phetsims / projectile-motion

"Projectile Motion" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
15 stars 13 forks source link

There have been many awkward formatting changes #315

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

Early on in @matthew-blackman and my work on the stats screen for Projectile Motion, many formatting changes snuck into the stats branch, and eventually made their way to master.

The suspect commit is https://github.com/phetsims/projectile-motion/commit/61fdbeff9528adbb4ef7ebe634ee9c01ced1228c#diff-a1da984b8bc78adeeed42ccb520714ff16d978a0ce46034be41e5edeee855047

and yields changes like so:

https://github.com/phetsims/projectile-motion/commit/61fdbeff9528adbb4ef7ebe634ee9c01ced1228c#diff-a1da984b8bc78adeeed42ccb520714ff16d978a0ce46034be41e5edeee855047R113

@matthew-blackman, I just wanted to you be aware of this. In my opinion the changes make the code worse and harder to read and maintain. Perhaps while converting to typescript over in https://github.com/phetsims/projectile-motion/issues/306 we have an opportunity to reformat back to the previous, more idiomatic, PhET code style.

Just good to keep in mind!

zepumph commented 1 year ago

I think one of the most awkward changes is this:

https://github.com/phetsims/projectile-motion/blob/28018349b02d76ac70f09da7c08b64ad08e94a2a/js/common/model/ProjectileMotionModel.js#L214-L225

I would prefer the final curly brace and closing paren to be on the same line. How about something like this:

Index: js/common/model/ProjectileMotionModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/ProjectileMotionModel.js b/js/common/model/ProjectileMotionModel.js
--- a/js/common/model/ProjectileMotionModel.js  (revision 2466c27637470e8b59420b79df69727602e16695)
+++ b/js/common/model/ProjectileMotionModel.js  (date 1670876386169)
@@ -214,15 +214,13 @@
     // @public {DerivedProperty.<number>}
     this.airDensityProperty = new DerivedProperty(
       [ this.altitudeProperty, this.airResistanceOnProperty ],
-      calculateAirDensity,
-      {
+      calculateAirDensity, {
         tandem: tandem.createTandem( 'airDensityProperty' ),
         units: 'kg/m^3',
         phetioDocumentation:
           'air density, depends on altitude and whether air resistance is on',
         phetioValueType: NumberIO
-      }
-    );
+      } );

     // --animation controls

I see this also was changed by that original commit. While this seems relative small potatoes, I do think it is worth some time to all get on the same page. Sorry it took me until now to recommend the change.

matthew-blackman commented 1 year ago

I agree this is awkward formatting, not sure how these unintended changes snuck in. I'm using 'Reformat code' with every change, and have linting set up. I can manually go through and fix these while converting to TS unless there is a more automated way. @zepumph thoughts?

zepumph commented 1 year ago

I don't know of an automated way. From the commit it looked like it was really really early on. No worries about it! I think it is worth thinking about when converting to typescript because you are already looking at those spots anyways. We don't need to get every case, but especially when it is atrociously harder to understand, let's clean those up.

matthew-blackman commented 1 year ago

I believe I got all of the formatting changes affected by https://github.com/phetsims/projectile-motion/commit/61fdbeff9528adbb4ef7ebe634ee9c01ced1228c#diff-a1da984b8bc78adeeed42ccb520714ff16d978a0ce46034be41e5edeee855047. Closing this as we will be revisiting each file in https://github.com/phetsims/projectile-motion/issues/306