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

Introduce isUndefined, deprecate defined #93

Merged
merged 3 commits into from
Mar 6, 2019
Merged

Introduce isUndefined, deprecate defined #93

merged 3 commits into from
Mar 6, 2019

Conversation

mgred
Copy link
Contributor

@mgred mgred commented Feb 28, 2019

Purpose (TL;DR) - mandatory

This PR introduces an assertion called isUndefined that passes when the given value is undefined. This also deprecates the defined assertion

Background (Problem in detail) - optional

See #87 #89

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm run test

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

@mgred
feat(isUndefined): add new assetion isUndefined
28a2705 
check if an actual value is undefined

See: #89 #87
@mgred mgred self-assigned this Feb 28, 2019
@mgred mgred changed the title Introduce is undefined Introduce isUndefined Feb 28, 2019
@mgred mgred changed the title Introduce isUndefined Introduce isUndefined, deprecate defined Feb 28, 2019
@mantoni
Copy link
Member

mantoni commented Feb 28, 2019

I understand "deprecated" as "prints a warning when used, but works as before". You removed it 😏

This is fine with me, it just makes it a necessity to replace all defined usages when upgrading while a warning allows to gradually introduce the change.

@coveralls
Copy link

coveralls commented Feb 28, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 308

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 304: 0.0%
Covered Lines: 659
Relevant Lines: 659
💛 - Coveralls

@mroderick
Copy link
Member

mroderick commented Feb 28, 2019 •
edited
Loading

I understand "deprecated" as "prints a warning when used, but works as before".

That is my understanding too.

I'd much prefer to give people a gentle warning, than break their tests immediately.

Perhaps first we can move https://github.com/sinonjs/sinon/blob/master/lib/sinon/util/core/deprecated.js t https://github.com/sinonjs/commons and then use it from there?

@mgred
Copy link
Contributor Author

mgred commented Feb 28, 2019

I understand "deprecated" as "prints a warning when used, but works as before". You removed it 😏

This is fine with me, it just makes it a necessity to replace all defined usages when upgrading while a warning allows to gradually introduce the change.

🤦‍♂️ You're right! Showing a message is the way better option here 😅

Do you think a simple console.warn("This assertion will be deprecated with the next major version! Use assert.isUndefined instead"); is enough here?

Thanks a lot for your thoughts and review 👍

@mgred
Copy link
Contributor Author

mgred commented Feb 28, 2019

I understand "deprecated" as "prints a warning when used, but works as before".

That is my understanding too.

👍

I'd much prefer to give people a gentle warning, than break their tests immediately.

Perhaps first we can move https://github.com/sinonjs/sinon/blob/master/lib/sinon/util/core/deprecated.js t https://github.com/sinonjs/commons and then use it from there?

Cool, thanks for pointing that out. I will make a PR in the @sinonjs/commons package and will continue here as you guys suggested

Thank you very much 🙌

mgred added a commit to mgred/commons that referenced this pull request Mar 1, 2019
@mgred
feat(deprecated): add new functionality to deprecate
1474b60 
move module to deprecate functions to commons

See: sinonjs/referee#93
@mgred mgred mentioned this pull request Mar 1, 2019
Merged
mgred added a commit to sinonjs/commons that referenced this pull request Mar 6, 2019
@mgred
feat(deprecated): add new functionality to deprecate (#13)
769ec34 
* feat(deprecated): add new functionality to deprecate

move module to deprecate functions to commons

See: sinonjs/referee#93

* test(deprecated): add more tests

* corrected typo

* format: add spacing
mroderick
mroderick approved these changes Mar 6, 2019
View reviewed changes
Copy link
Member

@mroderick mroderick left a comment

Choose a reason for hiding this comment

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

Looks great!

@mgred
Copy link
Contributor Author

mgred commented Mar 6, 2019

Cool 👍

Just for documentation purpose. In https://github.com/sinonjs/referee/pull/93/files#diff-560744c83c3809c6fed70c6edc2d433dR11 We need to wrap everthing up because assert chaecks that the passed function has at least an arity of one and throws if not.

@mgred mgred merged commit c8f8951 into master Mar 6, 2019
@mgred mgred deleted the introduce-is-undefined branch March 6, 2019 23:07
@mgred
Copy link
Contributor Author

mgred commented Mar 6, 2019

Oh I just merged 😕 and maybe a bit too fast? @mantoni anything from you side?

@mantoni
Copy link
Member

mantoni commented Mar 7, 2019

No worries. This looks good to me 👌

@mroderick
Copy link
Member

This has been published as @sinonjs/referee@3.2.0

@mantoni mantoni added this to the 4.0 milestone Mar 9, 2019
@mantoni mantoni mentioned this pull request Mar 9, 2019
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroderick mroderick mroderick approved these changes

@mantoni mantoni Awaiting requested review from mantoni

Assignees

@mgred mgred

Labels
None yet
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mgred @mantoni @coveralls @mroderick