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

[WIP] Upgrade braces #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

mobilutz
Copy link

@mobilutz
Copy link
Author

@jonschlinkert It looks like in a braces version between 0.1.4 and 2.3.1 the handling of one braces value has been changed. I am not too familiar with what expand-braces is used for, and if the failing specs are even worth to look at in detail.

Could you please take a look at this and give some feedback.

Thanks

@Teamop
Copy link

Teamop commented Feb 18, 2019

@jonschlinkert the cases failed mostly for brace with single item changes

--v1.8
// default
braces('a{b}c');
//=> ['abc']
// if with option
braces('a{b}c', {bash: true});
//=> ['a{b}c']

--v2
// default, seems no option to control this behavior
console.log(braces.expand('a{b}c'));
//=> ['a{b}c']

@mobilutz mobilutz mentioned this pull request Feb 18, 2019
@jonschlinkert
Copy link
Member

jonschlinkert commented Feb 18, 2019

the cases failed mostly for brace with single item changes

  1. This is correct behavior according to every convention for brace expansion that I've found, and it's in the unit tests
  2. This is completely unrelated to this issue. If you have a question or want to achieve something that you're not able to figure out from the docs, then please create a new issue and I'd be happy to help.

It looks like in a braces version between 0.1.4 and 2.3.1 the handling of one braces value has been changed.

Thank you for the PR @mobilutz! If it's not too much trouble, could you update the .travis.yml config to whatever versions are specified in the braces travis config, so that we can ensure unit tests are passing? Then I'll merge this in and publish a patch ASAP! Thanks!

(edit: and I will take care of fixing the tests)

@jonschlinkert jonschlinkert mentioned this pull request Feb 18, 2019
@micromatch micromatch deleted a comment from Teamop Feb 19, 2019
@jonschlinkert
Copy link
Member

jonschlinkert commented Feb 19, 2019

@Teamop I didn't read your last message, it was deleted. Please create a new issue as I requested in my last comment.

Also, if the unit tests are failing, it's because the unit tests are wrong in this library. Braces is correct per specification, and there are comprehensive unit tests as such. If you see something that is wrong in braces, and you can point to the specification that shows that the behavior is wrong, then please create an issue on braces.

@Teamop
Copy link

Teamop commented Feb 19, 2019

@jonschlinkert ok, not sure why it was deleted... thanks again

@Teamop
Copy link

Teamop commented Feb 19, 2019

@jonschlinkert ohh.. I just saw the recent message, so the test cases are wrong, that's the reason why the cases failed. I'm not familiar with braces expansion, so thanks for your explanation.

@mobilutz
Copy link
Author

mobilutz commented Feb 19, 2019

@jonschlinkert I updated .travis.yml to the same content as braces.

Now node 0.10 and 0.12 are failing completely because of mocha - the version 5.2.0 of mocha requires node >4: https://github.com/mochajs/mocha/blob/v5.2.0/package.json#L452
The rest show the same errors as before.

@mobilutz
Copy link
Author

@jonschlinkert I downgrade mocha to the same version of braces, and now all travis runs fail at the same points.

@jonschlinkert
Copy link
Member

jonschlinkert commented Feb 19, 2019 via email

@stramunin

This comment has been minimized.

@gregorycraske

This comment has been minimized.

@mobilutz
Copy link
Author

@gregorycraske You can use yarns resolutions to upgrade braces manually:
https://yarnpkg.com/lang/en/docs/selective-version-resolutions/

We have this in our package.json

  "resolutions": {
    "braces": "^2.3.2"
  }

Of course and upgrade hear would be better, but with this you can have a correct audit and secure system.

@hackeo1
Copy link

hackeo1 commented Feb 27, 2019

Any update with the travis tests?

@mobilutz
Copy link
Author

@jonschlinkert I am cleaning up all my old GitHub PullRequests and want to ask how I can support further here.

I currently cannot find the TravisCI runs at all, and don't know how I can push this change forward.

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

6 participants