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

Did you mean flag #6316

Closed
wants to merge 3 commits into from
Closed

Conversation

benglewis
Copy link

I can't seem to build the tests 100% on my Mac, so I'm opening this unable to 100% verify it :(

Ben Lewis added 2 commits June 30, 2018 16:11
@asterite
Copy link
Member

asterite commented Jul 2, 2018

Sorry, but this change makes no sense. What problem are you trying to solve?

@benglewis
Copy link
Author

@asterite #5291
I would have liked to provide the actual file name in the error, but that information isn't available, and fixing that issue would involve a major amount of re-write that could break other people's use cases, so I've avoided it thus far. Let me know if you see any other issues here :)

@asterite
Copy link
Member

asterite commented Jul 2, 2018

That issue is about showing suggestions for when you pass a flag to the compiler, and the flag is not found. Here you are changing option parser (which can be used in any crystal program, not just the compiler), to suggest that the flag should be passed Ina different way. And that makes no sense at all.

@straight-shoota
Copy link
Member

@benglewis OptionParser is used by countless Crystal applications, not just by the compiler. You can't add a very compiler-specific behaviour to such a general class. This did-you-mean flag needs to go into the compiler's command handling.

@benglewis
Copy link
Author

Oh.

@paulcsmith
Copy link
Contributor

@benglewis I've run into this myself, especially when I was just starting Crystal. I would have loved to have seen a nice error like the one you are suggesting so I wouldn't have to ask for help.

@straight-shoota or @asterite could you provide a link to the file or some other guidance so that this suggestion could be shown in the correct place? That may help @benglewis or someone else make the changes to add this feature in the right place

@straight-shoota
Copy link
Member

This is the place to look: https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/command.cr#L170

@asterite
Copy link
Member

The original proposal is that if you run a program with crystal foo.cr --flag and that flag doesn't exist in the program foo.cr, then you'd get an error saying you should invoke it like crystal foo.cr -- --flag. To me, that seems impossible to implement. Or, well, it's this PR, but it's not correct to show that error because you can run the program as . /foo once it's compiled.

And I wouldn't go down the route of adding a flag that is present if you run the program through crystal, just for this error message.

There should simply be an answer to this in stackoverflow.com, problem solved.

https://stackoverflow.com/questions/45117892/passing-cli-arguments-to-excutables-with-go-run

@paulcsmith
Copy link
Contributor

paulcsmith commented Jul 31, 2018

I didn't read the original issue so that was my mistake.

I don't think it is a high priority, but I do think that Crystal will be more loved if more helpful errors can be shown to users. It's such a delight when languages/libraries give me a hint that saves even just a few minutes.

So maybe it would be hard, but if someone adds it, I think it would be a great addition! Especially because people may not know what to google for.

My thought would be that it only applies when using the crystal tool and if you give it an option it doesn't understand it shows what it shows now, plus an additional help saying something like: "If you meant to access this flag in your program use -- #{options}. Much like what the author did, but just for the crystal command and not in OptionParser. Maybe that's what you already imagined, but I wanted to get it written down just in case.

What do you think? If someone does implement it, would it be worth adding to save time and give users that extra bit of help so they don't break their flow?

@asterite
Copy link
Member

As usual, if someone wants to implement it, go ahead. After all I'm no longer approving/merging PRs :-P

@asterite
Copy link
Member

Also note that if you have a program that just does ARGV[0], so it doesn't use OptionParser, then there's simply no way to detect this. And if you only add this knowledge to OptionParser, if you use another shard to parse options then it won't work (unless that shard also adds a similar solution).

So I don't think the original issue makes much sense. That's just my opinion, though, but again, this seems impossible to implement.

@RX14
Copy link
Contributor

RX14 commented Aug 1, 2018

Really, crystal foo.cr --flag should send --flag to foo.cr.

Options before the filename should go the compiler, options after should go to the file. As well as being less confusing, this also makes it possible to do #!/usr/bin/env crystal in scripts.

@benglewis
Copy link
Author

Thank you for the discussion guys.

I am in agreement with @RX14 here. I think that simply considering the order of the flags would be the most logical version to me as a user, but I do understand that that may not fit with the strategy for Crystal. If a conclusion can be reached on what to implement, I would be happy to try as a complete newbie to Crystal :)

@benglewis benglewis closed this Aug 4, 2018
@benglewis benglewis deleted the did-you-mean-flag branch August 4, 2018 20:49
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.

5 participants