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

Coverage #196

Merged
merged 7 commits into from Apr 9, 2021
Merged

Coverage #196

merged 7 commits into from Apr 9, 2021

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Jul 9, 2020

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New rule
  • Changes an existing rule
  • Add autofixing to a rule
  • Other, please explain:

Testing coverage

What changes did you make? (Give an overview)

Builds on #195 (so that there would be a passing tests/linting baseline)

  • Testing: Check coverage
  • Testing: Set thresholds for coverage (100%)
  • Testing: Get to 100% coverage
  • Refactoring: Internally rename isCallingBack to isCallback as per file name and usage

In the process, this PR includes refactoring to remove dead code (and thereby avoid the need for istanbul ignore):

  • removed code block from always-return which is not necessary given that "good" return/throw statements cannot occur within shortcut syntax (which expect expressions)

If you prefer that all of the code be refactored to avoid istanbul ignore, I can do so, but I think the other code was more reasonable as a sanity guard (i.e., in case the code were ever refactored, those checks may be necessary).

@brettz9
Copy link
Contributor Author

brettz9 commented Jul 9, 2020

I guess I need to wait until #195 may be merged into development, so I can refactor on top of that.

@brettz9 brettz9 force-pushed the coverage branch 3 times, most recently from 5f3b854 to b0af281 Compare February 11, 2021 07:25
@brettz9
Copy link
Contributor Author

brettz9 commented Feb 11, 2021

I've rebased this as well as #197 and #198 on latest development.

@brettz9 brettz9 force-pushed the coverage branch 3 times, most recently from 1a6661d to e70607a Compare February 11, 2021 15:31
jp7837 and others added 7 commits April 10, 2021 05:40
- Testing: Fix test (was not parsing)
- Testing: Fix test (missing an error message)
Also:
- internally renames `isCallingBack` to `isCallback` as per file name and usage
- removed code block from `always-return` which is not necessary given that "good" return/throw statements cannot occur within shortcut syntax (which expect expressions)
@brettz9
Copy link
Contributor Author

brettz9 commented Apr 9, 2021

Rebased. Note that this also currently adds changes of #184 / #188 (added to master)--IIRC, necessary for getting to full coverage and/or satisfying the linter. Also updates some devDeps. and the lock file (using npm@7 format, requiring devs working on the project to use npm@7)

@xjamundx
Copy link
Contributor

xjamundx commented Apr 9, 2021

I personally don't care about 100% test coverage, but if we have it, let's try to keep it :D

@xjamundx xjamundx merged commit 54c55b3 into eslint-community:development Apr 9, 2021
@brettz9 brettz9 deleted the coverage branch April 9, 2021 22:14
@brettz9
Copy link
Contributor Author

brettz9 commented Apr 9, 2021

Cool--Appreciate the flexibility! FWIW, besides giving some assurances that edge cases have been considered and are working as expected, I find another advantage is ensuring self-documentation for others about what will happen under those edge case conditions.

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.

None yet

3 participants