Sitelet https://github.com/fsprojects/Paket/pull/2969
Skip to content

Fixes a few things when framework: lines are parsed in template - #2969

Merged
forki merged 2 commits into
fsprojects:masterfrom
vbfox:template_framework_parsing_fixes
Jan 10, 2018
Merged

forki merged 2 commits into
fsprojects:masterfrom
vbfox:template_framework_parsing_fixes

Conversation

@vbfox

@vbfox vbfox commented Jan 10, 2018

Copy link
Copy Markdown
Contributor

Had to hunt some oddities due to that today 😉

  • Accept the framework:foo format (without a space after ":")
  • Fail on unknown frameworks instead of silently ignoring the framework and continuing as if nothing was specified

Note: I choose to fail hard using failwith instead of wiring the Choice<,> around as dependency parsing in getDependencyByLine already does that.

vbfox added 2 commits January 10, 2018 11:20
* Accept the framework:foo format (without a space after ":")
* Fail on unknown frameworks instead of silently ignoring
@enricosada

Copy link
Copy Markdown
Contributor

👍 ci error is unrelated

@forki
forki merged commit 4ca6157 into fsprojects:master Jan 10, 2018
@matthid

matthid commented Jan 10, 2018

Copy link
Copy Markdown
Member

Technically I think this is a breaking change, let’s see what happens...

@enricosada

Copy link
Copy Markdown
Contributor

silent was bad => bug.

be less strict on input file (hand edited by user) is not too bad.
i think is a situation of Generous on input, strict on output.

@vbfox
vbfox deleted the template_framework_parsing_fixes branch January 10, 2018 14:28
@vbfox

vbfox commented Jan 10, 2018

Copy link
Copy Markdown
Contributor Author

I consider it more of a bug fix even if there are some edge cases that it could break (Invalid file and the user never noticed and started to depend on invalid framework being merged with no-framework blocks)

@matthid

matthid commented Jan 10, 2018

Copy link
Copy Markdown
Member

I was just saying, I think we reverted a similar change before. Don’t get me wrong I like the change ;)
Also my memory might be bad

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.

4 participants