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

Make stack build --flag print warning when flag or package is unknown #617

Closed
k-bx opened this issue Jul 17, 2015 · 10 comments
Closed

Make stack build --flag print warning when flag or package is unknown #617

k-bx opened this issue Jul 17, 2015 · 10 comments

Comments

@k-bx
Copy link
Contributor

k-bx commented Jul 17, 2015

It can be quite dangerous when you make a mistake in package name (instead of "AdServer" I used lower-case "adserver"), and flag doesn't apply (and you try to deploy debug-build into production because of that). I would like stack to error or warn me about missing name.

@borsboom
Copy link
Contributor

Big +1 on this one. I've run into this myself on several occasions.

@DanBurton
Copy link
Contributor

+1 and I presume the same warning should go for the flags section of stack.yaml.

@k-bx
Copy link
Contributor Author

k-bx commented Jul 17, 2015

Btw, the more I think of it, the more I have an opinion that it should actually error, not warn. Why would anyone want a flag that's not declared in .cabal-file?

@DanBurton
Copy link
Contributor

I agree. This has the potential to run a lengthy erroneous build, and the warning could be easily missed, especially when the invocation of stack is automated.

+1 should be error. I don't think any sort of "force" option is necessary to bypass the error either; the solution is just to fix the flags.

@borsboom
Copy link
Contributor

Related: #611 (Should show error if setting --flag for package in the snapshot database).

@borsboom
Copy link
Contributor

Merging this and #611. The error/warning message should be adjusted in the case where the flag matches a snapshot package to inform the user that flags can't be set for those.

@snoyberg snoyberg self-assigned this Jul 21, 2015
@snoyberg
Copy link
Contributor

Self-assigning

@borsboom
Copy link
Contributor

I'm also inclined to agree it should be an error rather than a warning.

@snoyberg
Copy link
Contributor

OK, this is implemented and has a number of integration tests in place as well. Can others give it a whirl and see how it behaves?

@k-bx
Copy link
Contributor Author

k-bx commented Jul 22, 2015

@snoyberg I confirm this works, thank you!

@k-bx k-bx closed this as completed Jul 22, 2015
qrilka added a commit that referenced this issue Nov 16, 2018
This follows the changes appeared in #617
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants