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: --100 #332

Merged
merged 11 commits into from Oct 6, 2021
Merged

feat: --100 #332

merged 11 commits into from Oct 6, 2021

Conversation

@gr2m
Copy link
Contributor

@gr2m gr2m commented Oct 1, 2021

closes #329

@gr2m gr2m marked this pull request as draft Oct 1, 2021
yargs.middleware((argv) => {
if (!argv[100]) return argv

return {
...argv,
branches: 100,
functions: 100,
lines: 100,
statements: 100,
}
})
Copy link
Contributor Author

@gr2m gr2m Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code doesn't seem to be executed. I haven't worked with yargs middleware before but it seems like a good approach here? Do you know why the code is not executed?

Loading

Copy link
Owner

@bcoe bcoe Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable approach to me, and seems like it should work, perhaps:


yargs.middleware((argv) => {
    if (!argv['100']) return argv

    return {
      ...argv,
      branches: 100,
      functions: 100,
      lines: 100,
      statements: 100,
    }
}

I wouldn't expect the argument key itself to be coerced into an integer.

Loading

Copy link
Contributor Author

@gr2m gr2m Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my problem here is that the middleware is not applied at all. I think I'm missing something obvious. Any idea what it could be?

Loading

Copy link
Owner

@bcoe bcoe Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally, it seems like this is a bug that I fixed in yargs@17, at least it works as expected with yargs@17 ...

Unfortunately, I think we might want to hold off on dropping Node 10 until I can drop Node 10 on the libraries I manage at Google.

We can implement a less elegant version here:

https://github.com/bcoe/c8/blob/main/lib/commands/report.js#L29

Which I believe should work in the appropriate contexts (we'll want to confirm it works for the check-coverage command).

Let's add a // TODO: comment to switch to the middleware approach as soon as we can drop Node 10?

Loading

test/integration.js Outdated Show resolved Hide resolved
Loading
bcoe
bcoe approved these changes Oct 4, 2021
Copy link
Owner

@bcoe bcoe left a comment

This general approach looks good to me, let me know when you're ready for review 👍

Loading

yargs.middleware((argv) => {
if (!argv[100]) return argv

return {
...argv,
branches: 100,
functions: 100,
lines: 100,
statements: 100,
}
})
Copy link
Owner

@bcoe bcoe Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable approach to me, and seems like it should work, perhaps:


yargs.middleware((argv) => {
    if (!argv['100']) return argv

    return {
      ...argv,
      branches: 100,
      functions: 100,
      lines: 100,
      statements: 100,
    }
}

I wouldn't expect the argument key itself to be coerced into an integer.

Loading

lib/parse-args.js Outdated Show resolved Hide resolved
Loading
@gr2m gr2m marked this pull request as ready for review Oct 4, 2021
@bcoe bcoe merged commit 4205f2f into bcoe:main Oct 6, 2021
6 checks passed
Loading
@bcoe
Copy link
Owner

@bcoe bcoe commented Oct 6, 2021

@gr2m thank you for the contribution \o/

Loading

@gr2m gr2m deleted the 329/--100 branch Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants