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: Hard CLI error when Argu parse fails AND Argu switch detected. #1128

Conversation

bentayloruk
Copy link
Contributor

Don't want to merge this immediately, because this CLI issue is causing confusion. However, putting change here for comment. /cc #1072 #594 #1082 #1090.

@baronfel
Copy link
Contributor

I like this better than the more-destructive fix (that ended up not working) from #1301, because it educates users that there is a better syntax and can move them towards it.

@matthid
Copy link
Member

matthid commented Apr 25, 2017

We will finally get over this with FAKE 5 netcore version, which has a new CLI.

@matthid
Copy link
Member

matthid commented May 7, 2017

I think this is good. Just one thing: Can we add an additional warning when we use the old-style arguments (not as big as previously but still).

I know the previous PR has been reverted, but it really showed that some are not even noticing that they are using an "old" style of calling FAKE. Ideally they would see the warning and get a chance to improve the argu syntax or the new FAKE 5 Syntax before we release it.

@bentayloruk
Copy link
Contributor Author

Hi @matthid. I'm not sure if you comment was for me, but I've long assumed this PR was dead in the water. There were no comments on it originally and I saw work happening elsewhere. As a result, I've lost track of the CLI changes and have no idea what is coming in FAKE 5. Does your comment indicate otherwise, that this PR is still relevant?

@matthid
Copy link
Member

matthid commented May 8, 2017

@bentayloruk Yes I think its relevant and we should do something along your suggestion.
At least a single line of warning for using old-style arguments and a huge warning (as you suggest) when trying to use argu style comments instead of silently falling back.

Be warned though, this might have some edge cases and we might need to revert again.
One way would be to print the huge warning and exception, but still try to continue with quirk mode...

@bentayloruk
Copy link
Contributor Author

OK, cool. I wont be able to do anything about it this week, as I'm on holiday. I can have a look next week. Would that work?

@matthid
Copy link
Member

matthid commented May 8, 2017

Sure no need to hurry on that. Thanks!

@matthid matthid added the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label May 28, 2017
@bentayloruk
Copy link
Contributor Author

@matthid sorry for the delay. I'm in a place where I can have a look at this again. I'd like to help out and fix the confusion my Argu work initially caused. However, I've not been active in the FAKE discussions for a long time and am very concerned about making things worse. In addition, I see that there is Argu CLI debate going on over in Paket world, so it's a hot topic right now. What is the latest thinking here? If you can help assure me, I'll jump back on the bus!

* Fail build when Argu parse fails AND Argu switches detected. See fsprojects#1090.
* Fallback to old if parse fails and no Argu switches.
* Also as WIP function to map from old to new.  NOT TESTED YET OR IN
* USE.
* Add Argu to test project for tests.
Need Argu in the FakeCore test project, but paket install puts them in
the test projects as well.  Seems not to break anything, so committing.
@bentayloruk bentayloruk force-pushed the please-lord-help-me-end-the-cli-confusion-i-created branch from b402fae to 0ad77e5 Compare June 23, 2017 13:51
@bentayloruk
Copy link
Contributor Author

Rebased on latest master. It builds fine in VS, but I'm still struggling to get build.cmd working as per #1599, so not sure if tests will pass.

I still need to review this change, in the light of all the commits since I originally pushed it.

@agross
Copy link

agross commented Jun 29, 2017

What is the latest thinking here?

I was able to advertise Paket's new syntax while still supporting the old. I think was rather easy to achieve.

@bentayloruk
Copy link
Contributor Author

@agross sounds great. Could you point me to where you do that in Paket, so I can review?

@agross
Copy link

agross commented Jun 29, 2017

  1. You need to define old and new variants, the old ones should be hidden for --help output
  2. You need to TryGetResult the new an old variants
  3. For required args you need to check whether any variant has been specified
  4. I wrote some functions to ease that

@bentayloruk
Copy link
Contributor Author

Excellent, thanks for the details! I will have a look and consider it in relation to FAKE. Looks like Argu has some new capabilities I need to get up to speed on!

@matthid
Copy link
Member

matthid commented Jul 26, 2017

Just for your info: The reason why I still think this is important is because FAKE gets another CLI with the netcore version. So IMHO it makes sense starting with FAKE 5 to warn people when they use the pre-current version. Probably some time after FAKE 5 release it might make sense to warn for the current syntax as well and guide people into using the netcore version.

@matthid
Copy link
Member

matthid commented Feb 4, 2018

As FAKE 5 seems to come closer to its release I'm not sure it is worth breaking FAKE 4 at this time.

@matthid matthid closed this Feb 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement waiting for author Some information or action was requested and it needs to be addressed. Or a response from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants