Constraints on repo name should be enforced by grunt tasks.

[pixelzoom]

Over in https://github.com/phetsims/chipper/issues/1014, we bumped into repo name problems, reported by a 3rd party in the Google Group. Things got complicated, so @jonathanolson made the constraints explicit in https://github.com/phetsims/chipper/issues/1014#issuecomment-806228222.

Apparently those constraints are only explicit in documentation, and build tools are not checking them. And it's therefore still causing problems.

For example... grunt create-sim apparently doesn't enforce the constraints for sim repo name. So I can create a new sim repo, spend a lot of time working on it, and then find out that my repo name is invalid when grunt build fails. This happened to a 3rd-party today in https://groups.google.com/g/developing-interactive-simulations-in-html5/c/xDcqyvO0ABk. (The "solution" was "rename your repo".)

So here's

% cd chipper
% grunt create-sim --repo="big_whoop" --author="CM" --title="Big Whoop"
// Do many hours of work, then...
% cd big_whoop
% grunt build
Running "lint-all" task

/Users/cmalley/PhET/GitHub/big_whoop/js/big_whoop-main.js
  15:46  error  ["big_whoop"] is better written in dot notation  dot-notation

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

Warning: 1 errors and 0 warnings Use --force to continue.

Aborted due to warnings.

Also notice that not-so-helpful error message from grunt build. This is clearly not checking the repo name, and is failing somewhere downstream.

Anywhere that the --repo option is present, the constraints on repository name should be enforced. All grunt tasks, no exceptions.

[jonathanolson]

Anywhere that the --repo option is present, the constraints on repository name should be enforced. All grunt tasks, no exceptions.

This seems potentially over-zealous. I've patched createSim, however the above suggestion might be more likely to break things for 3rd party developers (if things are working). Can you clarify why this would be advantageous and worth the work?

[pixelzoom]

Why is it overzelaous to expect that the parsing for --repo should be handled in one place, instead of duplicated in many places, and in many different ways?

If by "overzealous" you mean "a lot of work to address something that was done poorly in the first place", then I agree.

[jonathanolson]

Why is it overzelaous to thing that the parsing for --repo should be handled in one place, instead of duplicated in many places?

Developer creates a repo that might not match the requirements but works with our toolchain currently (maybe... á-repo). The patch I made above won't break things for them. The changes you've described WILL break their entire workflow. So I believe putting a roadblock in the "creating things in a bad state" approach is good, but the "break everything and force everyone to retroactively change things" approach is what I mean by overzealous.

[pixelzoom]

@jonathanolson said:

Developer creates a repo that might not match the requirements but works with our toolchain currently

If it works, they are probably just lucky. If a 3rd-party is using PhET build tools, what possible value is there in allowing them to create a repo that doesn't meet the naming requirements for PhET build tools? The very last step in the chain is grunt build, and (as has been demonstrated repeatedly) it WILL NOT work with a repo name that doesn't meet the constraints, and in fact will likely fail downstream in an uncomprehensible way.

I said:

Why is it overzelaous to expect that the parsing for --repo should be handled in one place, instead of duplicated in many places, and in many different ways?

I'd like an answer to this. PhET now has a convention for repo name. Why is it desirable to have multiple/duplicate pieices of code that parse and verify the --repo argument?

[pixelzoom]

Discussed with @jonathanolson on Zoom. The benefits of enforcing the repo naming convention would seem to outweigh the potential of breaking things for 3rd partys, especially since grunt build doesn't work if the convention isn't followed. So @jonathanolson is going to add something this assertIsRepoName( repoName ) and use it where a repository name is provided as input.

[jonathanolson]

Done, and commented in the Google Group. @pixelzoom feel free to review or close.

[pixelzoom]

👍🏻 Closing.

[pixelzoom]

Reopening.

There's some thing wrong here. When the task below is forwarded to perennial, a perfectly legal repo name is not recognized. Example:

% pwd 
/Users/cmalley/PhET/GitHub/natural-selection
% grunt checkout-target --repo=natural-selection --target=1.3
Running "checkout-target" task
(Forwarding task to perennial)
Running "checkout-target" task
>> (node:20802) Warning: Accessing non-existent property 'padLevels' of module exports inside circular dependency
>> (Use `node --trace-warnings ...` to show where the warning was created)
Fatal error: Perennial task failed:
AssertionError [ERR_ASSERTION]: repo name should be composed of lowercase a-z characters, optionally with dashes used as separators
...

If I cd perennial this works fine. Which makes no sense, since the above says "Forwarding task to perennial".

[jonathanolson]

Copy-pasted this:

And:

Jonathans-MacBook-Pro:~/phet/git/perennial # grunt checkout-target --repo=natural-selection --target=1.3
Running "checkout-target" task
info: checking out shas for natural-selection 1.3
info: git checkout 1.3 on natural-selection
(node:1864) Warning: Accessing non-existent property 'padLevels' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
info: git pull on natural-selection
info: getting dependencies.json for natural-selection
info: checking out dependencies for natural-selection
info: git checkout 423d662a3672e3de42852ee6d6fdec705ac98dd5 on assert
info: git checkout 2f7d65c8d5614b162208c99e681bf660d902193c on axon
info: git checkout 0bbc0a38a8526ce83961f05afdd62c54c52d0598 on brand
info: git checkout 3ba49685d06015ed7c68444bd7f98a39f3376c12 on chipper
info: git checkout 1aa0a85545e7eda2a8bd1caca5d64bd8362d1bda on dot
info: git checkout 728803d851998c1c4a25792ddad31c8583010c2d on joist
info: git checkout 92bcdf54e64e8f0a8f6da4c8f67d7ca8b6d07a59 on kite
info: git checkout 1c57e441cce0cb5074855224ebc324dcbbcd4c70 on phet-core
info: git checkout cf5aa869c657b15d35b57e86f2a1e93917886086 on phet-io
info: git checkout 05a16aa5a556e216f58ebf786531c0c68724f877 on phet-io-client-guides
info: git checkout fc675191fbf7f3b96d0d84e08176f589fc5b7f2d on phet-io-wrappers
info: git checkout 6b667aab3691469e423abdf2fd7ad4d0ce7ad8df on phetcommon
info: git checkout c81f330b1c814f41a9d020529bd216470bd2fec6 on phetmarks
info: git checkout 090c44b0ba8e0a7effd2f6a7830f35eac92d59cc on query-string-machine
info: git checkout 671caccbf879c9efbe2c99f5d601256c329b716d on scenery
info: git checkout cd52a156dcb4d74cad0d8553ef24b6d4dfec6756 on scenery-phet
info: git checkout f6b2ee4d7039bdf1757dbbce88cfaa8b43dd667d on sherpa
info: git checkout be2c9e55d0de683bd9435376b95b35fc509cfa0e on studio
info: git checkout 692e3596652e9c58f53bf7e7071b64283f462be5 on sun
info: git checkout 0ec39d04fce75731fc76184e91b43e4b5bab9eb1 on tambo
info: git checkout bbce01446785be1b80a55b785d63a5619aeedb20 on tandem
info: git checkout 141f9a395def6a3e5da48cac968bcea70ee20331 on twixt
info: git checkout 30ffcbd69fd2e8a0b3f53386974812c5071b2860 on utterance-queue
info: npm update on natural-selection
info: npm update on chipper

Done.

[jonathanolson]

Ahhh! It's probably because the --repo flag is specified while IN the repo? Checking.

[jonathanolson]

When grunt.option is run and --repo is specified twice, it returns [ 'natural-selection', 'natural-selection' ]. Because it's an array, it fails validation.

[jonathanolson]

Is it acceptable to avoid --repo while running from a non-perennial repo? Otherwise we'll have to scan all the possible ways the repo could be specified in command-line options, validate AND remove.

[pixelzoom]

Is it acceptable to avoid --repo while running from a non-perennial repo?

I wouldn't call that "acceptable" programming -- you should never set something without checking whether it's already set. But I can live with it -- if the error message makes it possible to figure out what's going on. At the very least, print the bad repo in the message, so we can see that it's [ 'natural-selection', 'natural-selection' ].

[pixelzoom]

I.e., something like:

AssertionError [ERR_ASSERTION]: Bad value for --repo: [ 'natural-selection', 'natural-selection' ]. repo name should be composed of lowercase a-z characters, optionally with dashes used as separators

[jonathanolson]

Actually, I realized I can use the same grunt logic in the chipper side to check, I'll add the check.