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.

Sign up for GitHub

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

Jump to bottom

Fix e2e tests #385

Merged
merged 3 commits into from
Jan 23, 2023
Merged

Fix e2e tests #385

merged 3 commits into from
Jan 23, 2023

Conversation

juanpcapurro
Copy link
Contributor

@juanpcapurro juanpcapurro commented Jan 19, 2023 •
edited
Loading

see commit history for details. Main feedback items are:

  • is it okay to drop the table formatter? didn't drop it
  • should I backport the formatters' tests into this codebase? -- discussion deferred to: test the different formatters #386

@juanpcapurro juanpcapurro marked this pull request as draft January 19, 2023 20:54
@juanpcapurro
Copy link
Contributor Author

I'll get the table formatter back and remove the draft status. I realized that dropping it would involve a minor or perhaps even major version bump, which would prevent users getting the semver-compatible version from having the updated dependencies, and even if there's a bug in a formatter, it wouldn't have dire consequences.

@juanpcapurro juanpcapurro mentioned this pull request Jan 19, 2023
Closed
@juanpcapurro juanpcapurro marked this pull request as ready for review January 19, 2023 21:19
@fvictorio
Copy link
Contributor

Great work @juanpcapurro!

@frangio is the attribution here enough?

@dbale-altoros
Copy link
Collaborator

@juanpcapurro this is great!
Thanks for the contribution
As I remember in previous PR there was a discussion where to not lock the dependencies version and add the ^ in the package json to the ones previously had
I am in favor of that, considering what @frangio comment

@fvictorio are you agree ?
@juanpcapurro can you add those ?

@fvictorio
Copy link
Contributor

As I remember in previous PR there was a discussion where to not lock the dependencies version and add the ^ in the package json to the ones previously had

Good catch. Yeah, we should do that here too.

@frangio
Copy link

frangio commented Jan 20, 2023

I would take the LICENSE file from ESLint and put it in the formatters directory. Perhaps additionally a readme file in the same directory saying where the code was vendored from. That should do it.

@dbale-altoros
Copy link
Collaborator

@juanpcapurro do you want to finish it yourself ? (adding the ^ and what frangio said)
Or should I continue it ?

cheers! great job

@juanpcapurro
Copy link
Contributor Author

do you want to finish it yourself ? (adding the ^ and what frangio said)
Or should I continue it ?

I meant to do the semver-range dependencies in a separate PR and also take care of the dependencies for the package.json in the e2e directory, but there seems to be consensus to include them here so I will

I'll implement the suggested feedback sometime today or tomorrow

@dbale-altoros
Copy link
Collaborator

do you want to finish it yourself ? (adding the ^ and what frangio said)
Or should I continue it ?

I meant to do the semver-range dependencies in a separate PR and also take care of the dependencies for the package.json in the e2e directory, but there seems to be consensus to include them here so I will

I'll implement the suggested feedback sometime today or tomorrow

excellent!! thanks for your contribution

@juanpcapurro
vendor eslint formatters
dc15b13 
- copied the current version of eslint formatters into our codebase
  since they're no longer accesible from importing the `eslint` package
  (see discussions in protofire#380) for
  details
- only imported those already supported, there are many other eslint
  formatters in the current eslint version (8.32.0) than there were in
  the one we were using previously (5.6.0)
- explicity attribute the formatters' authors and include ESLint's
  license file.
- table formatter was dropped by the latest version of eslint, so I
  fetched it from 5.6.0 instead
- while building this commit I realized it was possible to remove a
  formatter entirely and all existing tests would pass, so I created an
  issue for discussing that
- made eslint a dev dependency to reduce install size, since it's only
  used for linting this codebase.
- added some dependencies of the eslint formatters. Used 'old'
  strip-ansi version 6.0.1 which seems to still be maintained instead of
  7.0.1 since from version 7 onwards it's a pure ESM package
@juanpcapurro
modify main executable so it works with commander updates
579121c 
- explicitly call .help() when no arguments are provided
- explicitly set name
- removed one of the ways to create a sample config file since it was
  dead code
- call .opts() to obtain the program's options, since they're no longer
  injected into the program object
@juanpcapurro
update dependencies
4bd4d1f 
- set dependencies of the root package to semver-match instead of exact
  match
- update outdated dependencies in the e2e subproject
- deleted yarn.lock file since it's not used an can confuse contributors
  on wether we use npm directly or yarn
- node 10 and 12 are both out of support, so I took the liberty of
  testing the packaga against 14 and 16 which are the two oldest
  supported ones.
@juanpcapurro
Copy link
Contributor Author

all feedback items should be addressed now. I took the liberty of changing the node version against which the CI is run to the two oldest supported versions (14,16) instead of 10 and 12, which had both reached end of life

@dbale-altoros
Copy link
Collaborator

all feedback items should be addressed now. I took the liberty of changing the node version against which the CI is run to the two oldest supported versions (14,16) instead of 10 and 12, which had both reached end of life

this is great... I approved the PR
@fvictorio
@frangio
Do you guys have something to add ?

@dbale-altoros dbale-altoros self-requested a review January 20, 2023 18:56
@frangio
Copy link

frangio commented Jan 20, 2023

Looks good to me! Thank you!

@dbale-altoros dbale-altoros merged commit 4ba86a7 into protofire:master Jan 23, 2023
@frangio
Copy link

frangio commented Jan 24, 2023

Do you plan to release this soon?

@dbale-altoros
Copy link
Collaborator

I will discuss it with Franco... we are addressing some of the issues in the repo

@juanpcapurro juanpcapurro mentioned this pull request Jun 13, 2023
Closed
juanpcapurro added a commit to solhint-community/solhint-community that referenced this pull request Jun 13, 2023
@juanpcapurro
add the 'compact' formatter
14b195c 
was removed when implementing protofire/solhint/issues/385 , I probably
missed it since it wasn't listed in the docs or command line options.
It's likely someone else will show up asking for some other eslint
formatter to be added back.

This adds it back from eslint @ 8.42.0.

I also ran our linter on it, so it's compliant with this codebase's
style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbale-altoros dbale-altoros dbale-altoros approved these changes

@fvictorio fvictorio Awaiting requested review from fvictorio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@juanpcapurro @fvictorio @dbale-altoros @frangio