zepumph
commented
2 years ago No preference here! I defer to what @marlitas thinks is the least confusing.
Closed marlitas closed 2 years ago
zepumph
commented
2 years ago No preference here! I defer to what @marlitas thinks is the least confusing.
chrisklus
commented
2 years ago JO: We were thinking of things like SimulaRasaColors and SimulaRasaConstants as types, but could see naming them all as camel case since they are objects LM: consistency seems good SR: CM has said something along the lines of: since we're accessing things like a static on a class, it's nice for them to be pascal case JG: If we went the lower case route, wouldn't that mean that all of our utils files etc should be changed too? MK: I don't remember this split, I was thinking we used Pascal case for objects too JO: In that case, should we make the strings file capitalized MK: Strings files are auto generated and could be easily changed. Going the opposite direction would be a lot of work. I think we should just go for it and start converting the strings files to pascal case.
@marlitas volunteered to make this change in simula rasa and update other repos, thanks! If it takes to long @marlitas will bring this back.
chrisklus
commented
2 years ago @samreid found this: Files should be named like CapitalizedCamelCasing.js when returning a constructor, or lowerCaseCamelCasing.js when returning a non-constructor function or singleton. When returning a constructor or singleton, the constructor name should match the filename. Where singletons are treated like classes with static attributes (like SimulaRasaConstants or SimulaRasaColors), uppercase should be used.
We think a strings file falls under the class example mentioned in the last sentence.
pixelzoom
commented
2 years ago Every repo uses camelCase for the strings file (see list below). And now simula-rasa is different.
More importantly... This is now causing a build failure (see https://github.com/phetsims/simula-rasa/issues/27) because the build tools require camelCase.
% ls */*Strings*
ls: */*Strings*: No such file or directory
cmalley@lydian[64]: pwd
/Users/cmalley/PhET/GitHub
cmalley@lydian[65]: ls */js/*Strings*
acid-base-solutions/js/acidBaseSolutionsStrings.ts
area-builder/js/areaBuilderStrings.ts
area-model-algebra/js/areaModelAlgebraStrings.ts
area-model-common/js/areaModelCommonStrings.ts
area-model-decimals/js/areaModelDecimalsStrings.ts
area-model-introduction/js/areaModelIntroductionStrings.ts
area-model-multiplication/js/areaModelMultiplicationStrings.ts
arithmetic/js/arithmeticStrings.ts
atomic-interactions/js/atomicInteractionsStrings.ts
balancing-act/js/balancingActStrings.ts
balancing-chemical-equations/js/balancingChemicalEquationsStrings.ts
balloons-and-static-electricity/js/balloonsAndStaticElectricityStrings.ts
bamboo/js/bambooStrings.ts
beers-law-lab/js/beersLawLabStrings.ts
bending-light/js/bendingLightStrings.ts
blackbody-spectrum/js/blackbodySpectrumStrings.ts
blast/js/blastStrings.ts
build-a-fraction/js/buildAFractionStrings.ts
build-a-molecule/js/buildAMoleculeStrings.ts
build-a-nucleus/js/buildANucleusStrings.ts
build-an-atom/js/buildAnAtomStrings.ts
bumper/js/bumperStrings.ts
buoyancy/js/buoyancyStrings.ts
calculus-grapher/js/calculusGrapherStrings.ts
capacitor-lab-basics/js/capacitorLabBasicsStrings.ts
center-and-variability/js/centerAndVariabilityStrings.ts
chains/js/chainsStrings.ts
charges-and-fields/js/chargesAndFieldsStrings.ts
circuit-construction-kit-ac-virtual-lab/js/circuitConstructionKitAcVirtualLabStrings.ts
circuit-construction-kit-ac/js/circuitConstructionKitAcStrings.ts
circuit-construction-kit-black-box-study/js/circuitConstructionKitBlackBoxStudyStrings.ts
circuit-construction-kit-common/js/circuitConstructionKitCommonStrings.ts
circuit-construction-kit-dc-virtual-lab/js/circuitConstructionKitDcVirtualLabStrings.ts
circuit-construction-kit-dc/js/circuitConstructionKitDcStrings.ts
collision-lab/js/collisionLabStrings.ts
color-vision/js/colorVisionStrings.ts
concentration/js/concentrationStrings.ts
coulombs-law/js/coulombsLawStrings.ts
counting-common/js/countingCommonStrings.ts
curve-fitting/js/curveFittingStrings.ts
density-buoyancy-common/js/densityBuoyancyCommonStrings.ts
density/js/densityStrings.ts
diffusion/js/diffusionStrings.ts
eating-exercise-and-energy/js/eatingExerciseAndEnergyStrings.ts
energy-forms-and-changes/js/energyFormsAndChangesStrings.ts
energy-skate-park-basics/js/energySkateParkBasicsStrings.ts
energy-skate-park/js/energySkateParkStrings.ts
equality-explorer-basics/js/equalityExplorerBasicsStrings.ts
equality-explorer-two-variables/js/equalityExplorerTwoVariablesStrings.ts
equality-explorer/js/equalityExplorerStrings.ts
estimation/js/estimationStrings.ts
example-sim/js/exampleSimStrings.ts
expression-exchange/js/expressionExchangeStrings.ts
faradays-law/js/faradaysLawStrings.ts
fluid-pressure-and-flow/js/fluidPressureAndFlowStrings.ts
forces-and-motion-basics/js/forcesAndMotionBasicsStrings.ts
fourier-making-waves/js/fourierMakingWavesStrings.ts
fraction-comparison/js/fractionComparisonStrings.ts
fraction-matcher/js/fractionMatcherStrings.ts
fractions-common/js/fractionsCommonStrings.ts
fractions-equality/js/fractionsEqualityStrings.ts
fractions-intro/js/fractionsIntroStrings.ts
fractions-mixed-numbers/js/fractionsMixedNumbersStrings.ts
friction/js/frictionStrings.ts
function-builder-basics/js/functionBuilderBasicsStrings.ts
function-builder/js/functionBuilderStrings.ts
gas-properties/js/gasPropertiesStrings.ts
gases-intro/js/gasesIntroStrings.ts
gene-expression-essentials/js/geneExpressionEssentialsStrings.ts
geometric-optics-basics/js/geometricOpticsBasicsStrings.ts
geometric-optics/js/geometricOpticsStrings.ts
graphing-lines/js/graphingLinesStrings.ts
graphing-quadratics/js/graphingQuadraticsStrings.ts
graphing-slope-intercept/js/graphingSlopeInterceptStrings.ts
gravity-and-orbits/js/gravityAndOrbitsStrings.ts
gravity-force-lab-basics/js/gravityForceLabBasicsStrings.ts
gravity-force-lab/js/gravityForceLabStrings.ts
greenhouse-effect/js/greenhouseEffectStrings.ts
griddle/js/griddleStrings.ts
hookes-law/js/hookesLawStrings.ts
interaction-dashboard/js/interactionDashboardStrings.ts
inverse-square-law-common/js/inverseSquareLawCommonStrings.ts
isotopes-and-atomic-mass/js/isotopesAndAtomicMassStrings.ts
john-travoltage/js/johnTravoltageStrings.ts
joist/js/joistStrings.ts
least-squares-regression/js/leastSquaresRegressionStrings.ts
make-a-ten/js/makeATenStrings.ts
masses-and-springs-basics/js/massesAndSpringsBasicsStrings.ts
masses-and-springs/js/massesAndSpringsStrings.ts
mean-share-and-balance/js/meanShareAndBalanceStrings.ts
mobius/js/mobiusStrings.ts
models-of-the-hydrogen-atom/js/modelsOfTheHydrogenAtomStrings.ts
molarity/js/molarityStrings.ts
molecule-polarity/js/moleculePolarityStrings.ts
molecule-shapes-basics/js/moleculeShapesBasicsStrings.ts
molecule-shapes/js/moleculeShapesStrings.ts
molecules-and-light/js/moleculesAndLightStrings.ts
my-solar-system/js/mySolarSystemStrings.ts
natural-selection/js/naturalSelectionStrings.ts
neuron/js/neuronStrings.ts
nitroglycerin/js/nitroglycerinStrings.ts
normal-modes/js/normalModesStrings.ts
number-line-common/js/numberLineCommonStrings.ts
number-line-distance/js/numberLineDistanceStrings.ts
number-line-integers/js/numberLineIntegersStrings.ts
number-line-operations/js/numberLineOperationsStrings.ts
number-play/js/numberPlayStrings.ts
ohms-law/js/ohmsLawStrings.ts
optics-lab/js/opticsLabStrings.ts
pendulum-lab/js/pendulumLabStrings.ts
ph-scale-basics/js/phScaleBasicsStrings.ts
ph-scale/js/phScaleStrings.ts
phet-io-test-sim/js/phetIoTestSimStrings.ts
plinko-probability/js/plinkoProbabilityStrings.ts
projectile-motion/js/projectileMotionStrings.ts
proportion-playground/js/proportionPlaygroundStrings.ts
quadrilateral/js/quadrilateralStrings.ts
ratio-and-proportion/js/ratioAndProportionStrings.ts
reactants-products-and-leftovers/js/reactantsProductsAndLeftoversStrings.ts
resistance-in-a-wire/js/resistanceInAWireStrings.ts
rutherford-scattering/js/rutherfordScatteringStrings.ts
scenery-phet/js/sceneryPhetStrings.ts
shred/js/shredStrings.ts
simula-rasa/js/SimulaRasaStrings.ts
states-of-matter-basics/js/statesOfMatterBasicsStrings.ts
states-of-matter/js/statesOfMatterStrings.ts
sun/js/sunStrings.ts
tambo/js/tamboStrings.ts
tangible/js/tangibleStrings.ts
tappi/js/tappiStrings.ts
trig-tour/js/trigTourStrings.ts
twixt/js/twixtStrings.ts
under-pressure/js/underPressureStrings.ts
unit-rates/js/unitRatesStrings.ts
vector-addition-equations/js/vectorAdditionEquationsStrings.ts
vector-addition/js/vectorAdditionStrings.ts
vegas/js/vegasStrings.ts
vibe/js/vibeStrings.ts
wave-interference/js/waveInterferenceStrings.ts
wave-on-a-string/js/waveOnAStringStrings.ts
waves-intro/js/wavesIntroStrings.ts
wilder/js/wilderStrings.ts
xray-diffraction/js/xrayDiffractionStrings.tsIf we want to consider renaming the strings file everywhere, and change the build tools, and possibly change other things that read the strings file (website?), we can certainly do that. But I don't feel that's necessary or a good use of time. Until then, this change needs to be reverted.
pixelzoom
commented
2 years ago Also note that if I remove SimulaRasaString.ts, grunt modulify generates simulaRasaStrings.ts. So that's another grunt task that would need to be changed.
marlitas
commented
2 years ago I have on my todo list to change all of the other repos as well. I just got to Simula Rasa the other day. I plan on continuing to convert repos this week, but should I not do that anymore?
pixelzoom
commented
2 years ago I didn't realize that the plan was to change all repos. As you change them, builds are going to fail like https://github.com/phetsims/simula-rasa/issues/27. Is it OK for builds to fail until this change is completed? Is there anything else that is expecting camelCase and will break? (like grunt modulify) Does this change affect our ability to do maintenance releases? ... Again, it doesn't seem worth it to me.
marlitas
commented
2 years ago @chrisklus and I did some research/discovery on this.
We found the bug that caused this error in the build tools. A potential fix is in this patch:
We cannot determine on our own if this is worth the effort. It took us quite some time to determine the exact locations strings needed to be manipulated to convert to PascalCase, and we found no ways to make this backwards compatible.
Again, it doesn't seem worth it to me.
We are also starting to question if it is worth it.
Would like to send back to dev team for further discussion.
samreid
commented
2 years ago At today's dev meeting,
@pixelzoom elaborated we would need to change the build tools.
@zepumph and @samreid affirmed that they already thought that was in the scope of work.
@zepumph identifies that we do not want to go through a phase where things are broken for a while. @marlitas: Would it have to be able to read both styles? @samreid: It should move over all at once, so it never needs to support both styles at once.
@zepumph this work is worthwhile, and it should be done in a way that @marlitas is comfortable and also in a way that doesn't temporarily break builds.
@marlitas says it is a lot of manual work. Can this be automated? How will we automatically rename all the usages in sim code? A script?
Should we spend the time on renaming the string files? AV CM: Not now, we have other critical things happening. If we do this at all, it should be much later. CK JB (out today) JG - I don’t feel strongly about this and wouldn’t recommend spending time on it. IDE typically automatically imports this file. JO - Perhaps we’ll need to reclone, if we do something more than a case change it might be easier KP LM - Sounds like it would be good to do it at some point. We should strive for consistency. LV MK - same opinion as I have always had. I defer to new developers if they think it is important for consistency. MP MSL SR: It seems worthwhile to me, when the time is right. I would hope that it would take <6 hours by developing appropriate scripts. MB KP: I feel it is worth investigating scripts. Working on it soon sounds good.
@pixelzoom: Please notify us right away when this is committed.
Team consensus: Let's see if script/automation will make this feasible in the near future. @marlitas and @chrisklus will collaborate.
samreid
commented
2 years ago I reverted the commit for now since there is a build error on CT:
~/apache-document-root/main/simula-rasa$ grunt --brands=phet
Running "lint-all" task
[........................................] 100.00%
Running "report-media" task
Running "clean" task
Running "build" task
>> tsc complete: 1821ms
Building runnable repository (simula-rasa, brands: phet)
Building brand: phet
>> Webpack build complete: 1840ms
>> Unused string: key=simula-rasa.title, value={{TITLE}}
Fatal error: Perennial task failed:
AssertionError [ERR_ASSERTION]: missing entry for sim title, key = SIMULA_RASA/simula-rasa.title
at module.exports (/Users/samreid/apache-document-root/main/chipper/js/grunt/buildRunnable.js:152:3)
at async /Users/samreid/apache-document-root/main/chipper/js/grunt/Gruntfile.js:306:11
at async wrap (/Users/samreid/apache-document-root/main/chipper/js/grunt/Gruntfile.js:65:7)
Full Error details:
AssertionError [ERR_ASSERTION]: missing entry for sim title, key = SIMULA_RASA/simula-rasa.titleThanks. Since this is reverted, I'll close https://github.com/phetsims/simula-rasa/issues/27.
chrisklus
commented
2 years ago @marlitas and I did a small investigation this afternoon and came up with some steps to do this:
git mv all string files from camel case to pascal case.samreid
commented
2 years ago Could steps 1-2 be accomplished via a script that runs git mv? This would also retain history nicely.
chrisklus
commented
2 years ago @marlitas and I think this is ready for review - I randomly selected @zepumph to please take a look at your convenience.
We followed the steps in https://github.com/phetsims/simula-rasa/issues/26#issuecomment-1233533972 for this work.
I think the only code changes that weren't renames were in this commit: https://github.com/phetsims/chipper/commit/13f5fa5a71e52ba5563a365da28c760bc70901a7
The rest were just updating usages and renaming files. The script that did the git mv is in this commit: https://github.com/phetsims/perennial/commit/55b152aec50358e85278848d9856754d0d34ecd2
Hopefully this is a quick review - let us know if you have questions!
zepumph
commented
2 years ago Lots of changes, marking as blocking.
zepumph
commented
2 years ago I factored out the code for making pascalCase strings in chipper. Here I saw that the implementation working as expected:
pascalCase( '1000 fjk-TRI_ohHello')
'1000FjkTriOhHello'I also looked through all usages of \b[a-z]\w+Strings\b and things look good.
I poked through CT and didn't see anything related to this.
I can't think of anything else. Thanks for taking care of this!
marlitas
commented
2 years ago Thanks for the review and improvements @zepumph! Closing this issue as completed.
@samreid and I discussed the variety of naming conventions for
simulaRasaStrings(camelCase),SimulaRasaColors(PascalCase), andSimulaRasaConstants(PascalCase).I have been very confused by the difference, and we could not find a clear paper trail as to why
simulaRasaStringsis camelCase instead of PascalCase like the others.Here is where we decided that strings would be camelCase: https://github.com/phetsims/chipper/issues/922#issuecomment-606282425 Here is where we decided that colors would be PascalCase: https://github.com/phetsims/simula-rasa/issues/20
As a new dev I run into importing these files incorrectly over and over again. The lack of consistency is a big part of this problem. I would like to investigate the possibility of making the naming conventions for these files consistent.