phetsims / a11y-research

a repository to track PhETs research into accessibility, or "a11y" for short
MIT License
3 stars 0 forks source link

a11y-research violates PhET repo-name conventions and causes grunt tasks to fail. #172

Closed samreid closed 1 year ago

samreid commented 2 years ago

phettest shows a11y-research status as "failed".

I tried to diagnose, and I found that https://bayes.colorado.edu/dev/phettest-server/same-as-remote-master?repo=a11y-research reports {"output":"Invalid repo name","success":false}.

On bayes, there may be a permissions problem like so:

[phet-admin@bayes phettest]$ cd a11y-research/
[phet-admin@bayes a11y-research]$ git status
# On branch master
nothing to commit, working directory clean
[phet-admin@bayes a11y-research]$ git pull
Already up-to-date.
[phet-admin@bayes a11y-research]$ git push
remote: Permission to phetsims/a11y-research.git denied to phet-dev.
fatal: unable to access 'https://github.com/phetsims/a11y-research.git/': The requested URL returned error: 403

Does a11y-research need to be added to the new GitHub groups?

pixelzoom commented 2 years ago

See https://github.com/phetsims/chipper/issues/1014#issuecomment-806228222 for why the current repo-name conventions were established.

And from new-repo-checklist.md:

{{REPO}} - the repository name, e.g. gas-properties. This should be composed of lower-case characters and dashes, and should have <=214 characters (see naming constraints in https://docs.npmjs.com/cli/v7/configuring-npm/package-json).

I don't see anything in https://docs.npmjs.com/cli/v7/configuring-npm/package-json that disallows numbers. Perhaps that's a constraint that @jonathanolson added, and perhaps it can be relaxed.

samreid commented 2 years ago

It seem the name check was added my @zepumph:

function validateSimName( simName ) {

  // validate that it is lower-case with hyphens
  for ( let i = 0; i < simName.length; i++ ) {
    const charCode = simName.charCodeAt( i );

    // TODO: use a perennial data list as a white list, https://github.com/phetsims/special-ops/issues/170
    if ( charCode !== '-'.charCodeAt( 0 ) && ( charCode < 'a'.charCodeAt( 0 ) || charCode > 'z'.charCodeAt( 0 ) ) ) {
      return false;
    }
  }
  return true;
}

I agree it should probably allow numbers. @oliver-phet and I aren't sure why it was written using 'charCode`

emily-phet commented 1 year ago

I'm assuming this is no longer causing an issue. Closing.