Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint: fix issues #547

Closed
DavidGOrtega opened this issue May 20, 2021 · 12 comments · Fixed by #554 or #570
Closed

lint: fix issues #547

DavidGOrtega opened this issue May 20, 2021 · 12 comments · Fixed by #554 or #570
Assignees
Labels
technical-debt Refactoring, linting & tidying testing Unit tests & debugging

Comments

@DavidGOrtega
Copy link
Contributor

I open this issue to discuss CML's coding convention change and tasks needed to perform such change.

@0x2b3bfa0 @casperdcl @shcheklein

@DavidGOrtega DavidGOrtega added the technical-debt Refactoring, linting & tidying label May 20, 2021
@DavidGOrtega
Copy link
Contributor Author

This is also very unfriendly to review
image

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented May 20, 2021

It would be nice to have a parent issue in a higher-level repository — or a Slack thread[1, 2] — to discuss this with the other project teams so all we follow the same conventions. In our case, the specifics probably boil down to these three points:

  1. TypeScript or JavaScript
  2. dromedaryCase or snake_case
  3. Formatter and linter; see lint: prettier vs eslint setup-dvc#22

@DavidGOrtega
Copy link
Contributor Author

@0x2b3bfa0 of course but this issue is also the issue where to review what we are going to do once we agree.

@casperdcl
Copy link
Contributor

2 & 3. AFAIK everyone's agreed on camelCase. I'll fix the linter issues.

I think we can discuss 1. TS vs JS in a separate issue if needed

@casperdcl casperdcl changed the title CML coding conventions change lint: fix issues May 20, 2021
@casperdcl casperdcl added the testing Unit tests & debugging label May 20, 2021
@casperdcl
Copy link
Contributor

btw this yargs/yargs#1679 is annoyingly not fixed

@0x2b3bfa0

This comment has been minimized.

@0x2b3bfa0
Copy link
Member

By the way: do we agree on the definition of camel case? 😉

@casperdcl
Copy link
Contributor

Yes, but beware that the example table is not complete (e.g. it doesn't properly show rule 3b with a properNounObjectName rather than a ProperNounClassName)

@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented May 20, 2021

Renaming should be as easy (or as difficult) as taking a look to this list and applying the conversion rules above.

npx grasp --only-matching --no-filename --no-line-number 'ident' **/* |
grep --invert-match '[A-Z]' | grep '_' |
sort --unique

Roughly like this:

npx grasp --only-matching --no-filename --no-line-number 'ident' **/* |
grep '_' | grep -v '[A-Z]' | sort --unique |
while read identifier; do
  echo "$identifier => $(sed --regexp-extended 's/_([a-z])/\U\1/g' <<< "$identifier")"
done

@casperdcl
Copy link
Contributor

No, it's a fair bit more involved than that but I'll open a PR soon.

@casperdcl
Copy link
Contributor

lol that's about style which isn't currently tested so for a different issue/PR. In any case it's also self-inconsistent:

  1. descriptive
  2. within reason
  3. no saving horizontal space if it reduces understandability, e.g. no ambiguous or unfamiliar abbrevs
  4. [dumb, inconsistent rule] do not abbrev by removing letters in the middle of a word at all ever

Rule 4 can get lost because it's not always (2) within reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical-debt Refactoring, linting & tidying testing Unit tests & debugging
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants