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

Stricter parsing in Requirements.parseRestrictionsLegacy #2816

Merged
merged 1 commit into from Oct 4, 2017

Conversation

Projects
None yet
2 participants
@csmager
Contributor

csmager commented Oct 2, 2017

This more strictly requires expressions matching the syntax in the examples below:

Value Result
net45 Exactly net45
= net45 Exactly net45
>= net45 At least net45
>= net45 <= net46 Between net45 and net46
portable-windows8+net45+wp8 At least Profile78
>= portable-windows8+net45+wp8 At least Profile78

Note that due to expectations in lock file parsing tests like this one, we also accept >= net45 < net46 as meaning 'Between net45 and net46'.

Fixes #2803

@forki forki merged commit 93c02b2 into fsprojects:master Oct 4, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 4, 2017

Member

This lead to #2822 so I reverted for now. Let's investigate if this was intentional

Member

forki commented Oct 4, 2017

This lead to #2822 so I reverted for now. Let's investigate if this was intentional

@csmager

This comment has been minimized.

Show comment
Hide comment
@csmager

csmager Oct 4, 2017

Contributor

Sure. Would guess the code I didn’t touch only parses ‘framework’ and passes ‘s: net45’ along to be parsed.

I guess we need to decide whether we want to allow ‘frameworks’ or not. Either way, it needs a better error message I think.

Contributor

csmager commented Oct 4, 2017

Sure. Would guess the code I didn’t touch only parses ‘framework’ and passes ‘s: net45’ along to be parsed.

I guess we need to decide whether we want to allow ‘frameworks’ or not. Either way, it needs a better error message I think.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 4, 2017

Member
Member

forki commented Oct 4, 2017

@csmager

This comment has been minimized.

Show comment
Hide comment
@csmager

csmager Oct 4, 2017

Contributor

Agreed. Will add a few more tests with ‘framework’ with and without various combinations of ‘s’ and ‘:’

Contributor

csmager commented Oct 4, 2017

Agreed. Will add a few more tests with ‘framework’ with and without various combinations of ‘s’ and ‘:’

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 4, 2017

Member
Member

forki commented Oct 4, 2017

@csmager

This comment has been minimized.

Show comment
Hide comment
@csmager

csmager Oct 5, 2017

Contributor

I've checked this out, was as I expected - we find 'framework', read the text after it, remove any occurrence of : and then pass that along to the parser. In the case of #2822, we would try to parse s net40, and s isn't a valid framework identifier. In the previous 'loose' parsing, we would assume the s was = and parse net40.

That also means that e.g. frameworks: >= net40 and frameworks: auto-detect wouldn't have worked in the past.

I've made a change that will check for frameworks and then framework, which means they are now interchangeable. Will open a new PR shortly.

Contributor

csmager commented Oct 5, 2017

I've checked this out, was as I expected - we find 'framework', read the text after it, remove any occurrence of : and then pass that along to the parser. In the case of #2822, we would try to parse s net40, and s isn't a valid framework identifier. In the previous 'loose' parsing, we would assume the s was = and parse net40.

That also means that e.g. frameworks: >= net40 and frameworks: auto-detect wouldn't have worked in the past.

I've made a change that will check for frameworks and then framework, which means they are now interchangeable. Will open a new PR shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment