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

feat: add --version flag for CLI #308

Merged
merged 1 commit into from
Jul 10, 2024
Merged

feat: add --version flag for CLI #308

merged 1 commit into from
Jul 10, 2024

Conversation

ajitzero
Copy link
Contributor

Fixes #302

@ajitzero
Copy link
Contributor Author

Hi @isaacs - I had a couple of questions as this repo doesn't have a contributing guide.

  • Do I need to add tests for --version? I didn't see one for --help so I didn't add one for the version.
  • I've used the Commitizen format for commit messages. Should I squash them with a different format?

@isaacs
Copy link
Owner

isaacs commented May 24, 2024

Hi, @ajitzero thanks!

Yes, please add a test for the new cli flag. Should be added to ./test/bin.ts. (That's where the --help output is tested as well.)

I think it's best to squash commits prior to landing, at least to a "one commit per thing" kind of format, where tests pass on all commits, etc. But of course, sometimes it's easier to review in a more granular state. If you wanna squash it, great, if not, I can easily do it on my end when landing.

@isaacs
Copy link
Owner

isaacs commented May 24, 2024

If you rebase on main and re-run npm install ; npm test, it'll be clearer which test is failing because of the lacking coverage. I just dropped 16 and 21 from the CI, and added 22, so once it's green, we're good to go.

@ajitzero
Copy link
Contributor Author

ajitzero commented Jun 8, 2024

Hi @isaacs - sorry, I just got time again to look at this.

Asserts:  50232 pass  28 fail  50260 of 50260 complete
Suites:      16 pass   2 fail        18 of 18 complete

I see the above results for the main branch itself, and appears to be broken with (CI 167)

Let's cover this with a different PR first and hold off on merging this one (even though it's unlikely to make a difference).

@ajitzero
Copy link
Contributor Author

ajitzero commented Jun 8, 2024

Just added a test for --version.

I've also added /.tap to .gitignore - It only shows up when the tests fail.

@ajitzero ajitzero changed the title Add --version flag for CLI feat: add --version flag for CLI Jun 8, 2024
@ajitzero
Copy link
Contributor Author

ajitzero commented Jun 8, 2024

Let's cover this with a different PR first and hold off on merging this one (even though it's unlikely to make a difference).

I created a separate issue (#310) to track this.

We can merge this PR as it isn't relevant to the problem.

PR-URL: #308
Credit: @ajitzero
Close: #308
Reviewed-by: @isaacs
@isaacs isaacs changed the base branch from main to p July 10, 2024 17:11
@isaacs isaacs merged commit 6de86bf into isaacs:p Jul 10, 2024
20 of 24 checks passed
@ajitzero ajitzero deleted the feat/add-version-cli branch July 10, 2024 18:38
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.

What's the command to check the version?
2 participants