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

build: eslint, tslint, prettier consolidation, purge from non-root pkgs #3071

Conversation

petermetz
Copy link
Contributor

  1. We had dozens of different versions of ESLint, TSLint, Prettier
    declard in package.json files. Most versions were so old that they had
    ben EOL for several years too.
  2. The presence of these and their scripts/config files/etc were making
    it impossible to run the linter on the sub-folders where these were
    being used.
  3. As part of the big push to consolidate linting and code formatting on
    the entire project, this is phase 1 where I purged the mentioned dependencies
    from the project and now only the root package.json file declares them
    which is currently using the latest and greatest versions.
  4. I know that the diff is huge in this pull request but there are no
    code changes that would cause bugs or logic modification at all. It's all
    just applying automated formatting and linting on sub-directories where
    previously this was impossible. E.g., this entire change should not cause
    any sort of bugs (but of course we'll see what the CI execution has to
    say about that).
  5. The follow-up phases will consist of slowly applying more brazen
    refactoring in a much more targeted and surgical manner with small
    pull requests that are easy and quick to review.
  6. The smaller pull requests will target sub-directories that we still
    cannot include in the linting (see the .eslintignore file for a list) and
    then culminate in us finally enabling the linter for everything.
  7. The root folder's ESLint ignore file was refactored in a way that the
    files that we plan on fixing later are now ignored on a one by one basis
    instead of using patterns. This makes the ignore file a collection of
    to-dos where one can delete a single line from the ignore file (pertaining
    to a single file that is being ignored) and then proceed to fix the linter
    issues and send a small, easy to review pull request that does not overwhelm
    the maintainers when they are trying to review a huge diff with a large
    batch of linter fixes. Phase two of this project is then to one by one
    drain the ignore entries of these mentioned files from the ESLint ignore
    file.

So, the bottom line is that this might look like a hugely disruptive
change, but I've done my best to keep it as simple as possible so that
we don't have to worry about reviewing it line by line and instead just
focus on the big picture and the principle idea(s) behind it (code quality).

If it does end up introducing a bug or CI breakage, I'll volunteer to fix
those because I'm very confident that even if this does happen, it will
be very minor and quick fixes necessary only so I don't mind being on the
hook for it.

Signed-off-by: Peter Somogyvari [email protected]

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

@outSH
Copy link
Contributor

outSH commented Mar 8, 2024

@petermetz Weaver tests are failing

@petermetz
Copy link
Contributor Author

@petermetz Weaver tests are failing

@outSH Oops, thank you, I'm working on a fix!

cc: @sandeepnRES @VRamakrishna

@petermetz petermetz force-pushed the build-eslint-prettier-consolidation-phase-1 branch 3 times, most recently from b87fc2b to 10e39e4 Compare March 11, 2024 03:06
@petermetz
Copy link
Contributor Author

@outSH Weaver tests are now passing!

1. We had dozens of different versions of ESLint, TSLint, Prettier
declard in package.json files. Most versions were so old that they had
ben EOL for several years too.
2. The presence of these and their scripts/config files/etc were making
it impossible to run the linter on the sub-folders where these were
being used.
3. As part of the big push to consolidate linting and code formatting on
the entire project, this is phase 1 where I purged the mentioned dependencies
from the project and now only the root package.json file declares them
which is currently using the latest and greatest versions.
4. I know that the diff is huge in this pull request but there are no
code changes that would cause bugs or logic modification at all. It's all
just applying automated formatting and linting on sub-directories where
previously this was impossible. E.g., this entire change should not cause
any sort of bugs (but of course we'll see what the CI execution has to
say about that).
5. The follow-up phases will consist of slowly applying more brazen
refactoring in a much more targeted and surgical manner with small
pull requests that are easy and quick to review.
6. The smaller pull requests will target sub-directories that we still
cannot include in the linting (see the .eslintignore file for a list) and
then culminate in us finally enabling the linter for everything.
7. The root folder's ESLint ignore file was refactored in a way that the
files that we plan on fixing later are now ignored on a one by one basis
instead of using patterns. This makes the ignore file a collection of
to-dos where one can delete a single line from the ignore file (pertaining
to a single file that is being ignored) and then proceed to fix the linter
issues and send a small, easy to review pull request that does not overwhelm
the maintainers when they are trying to review a huge diff with a large
batch of linter fixes. Phase two of this project is then to one by one
drain the ignore entries of these mentioned files from the ESLint ignore
file.

So, the bottom line is that this might look like a hugely disruptive
change, but I've done my best to keep it as simple as possible so that
we don't have to worry about reviewing it line by line and instead just
focus on the big picture and the principle idea(s) behind it (code quality).

If it does end up introducing a bug or CI breakage, I'll volunteer to fix
those because I'm very confident that even if this does happen, it will
be very minor and quick fixes necessary only so I don't mind being on the
hook for it.

Signed-off-by: Peter Somogyvari <[email protected]>
@petermetz petermetz force-pushed the build-eslint-prettier-consolidation-phase-1 branch from 10e39e4 to 3546153 Compare March 12, 2024 14:44
@petermetz petermetz merged commit e4a8a66 into hyperledger-cacti:main Mar 12, 2024
123 of 144 checks passed
@petermetz petermetz deleted the build-eslint-prettier-consolidation-phase-1 branch March 12, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants