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

fix(plugin-version): Correctly check void releases for all falsy values #1536

Merged
merged 1 commit into from Jul 5, 2020

Conversation

martinjlowm
Copy link
Contributor

@martinjlowm martinjlowm commented Jul 1, 2020

js-yaml implements the YAML spec and it has no undefined type:

I.e.:

> require('js-yaml').safeLoad('')
undefined // if the file is empty
> require('js-yaml').safeLoad('releases:\n')
{ releases: null }

@arcanis
Copy link
Member

arcanis commented Jul 4, 2020

Indeed! Probably should be != null then - we still want to cover the case where the releases field simply doesn't exist.

js-yaml implements the YAML spec and it has no undefined type:

- nodeca/js-yaml#356 (comment)

I.e.:

```
> require('js-yaml').safeLoad('')
undefined
> require('js-yaml').safeLoad('releases:\n')
{ releases: null }
```
@martinjlowm martinjlowm changed the title fix(plugin-version): Correctly check void releases for a null value fix(plugin-version): Correctly check void releases for all falsy values Jul 5, 2020
@martinjlowm
Copy link
Contributor Author

martinjlowm commented Jul 5, 2020

Certainly, I don't know why I didn't realize the previous undefined check was for an absent key.

I've changed it to check for all falsy values (and an empty file which returns undefined, hence the optional operator), since I don't think '' nor 0 make sense either. I'm happy to make it less strict if you feel that is the better solution.

@arcanis arcanis merged commit d7ebacd into yarnpkg:master Jul 5, 2020
@arcanis
Copy link
Member

arcanis commented Jul 5, 2020

Thanks!

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

2 participants