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

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

Merged
merged 2 commits into from
Jan 10, 2018

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Jan 10, 2018

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
Collaborator

👍 ci error is unrelated

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

matthid commented Jan 10, 2018

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

@enricosada
Copy link
Collaborator

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
Copy link
Contributor Author

vbfox commented Jan 10, 2018

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
Copy link
Member

matthid commented Jan 10, 2018

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