search

vlang / v

Simple, fast, safe, compiled language for developing maintainable software. Compiles itself in <1s with zero library dependencies. Supports automatic C => V translation. https://vlang.io
MIT License
35.5k stars 2.15k forks source link

pref: fix regression of command flags not working #21647

Closed ttytm closed 2 weeks ago

ttytm commented 3 weeks ago

Fixes a regression where it's not possible to run some V commands with flags anymore after #21391. For example, if there is directory with the same name as the command, like having a fmt directory in the in the cwd and trying to run v fmt -diff file.v.

❯ v fmt -diff file.v
Unknown argument `-diff` for command `fmt`
JalonSolov commented 3 weeks ago

What happens if you have a fmt directory that you want to compile?

How about v run fmt?

In other words, if there's a directory with the same name as a command, it is limiting to force one over the other.

ttytm commented 3 weeks ago

What happens if you have a fmt directory that you want to compile?

Requires

v fmt/

Ending slash

JalonSolov commented 3 weeks ago

So force people to type the / at the end, when they don't normally need to do that.

ttytm commented 3 weeks ago

Yep.

How else would you solve that?

If you have a builtin v command and there is a directory with the same name?

JalonSolov commented 3 weeks ago

Ok, try this. Suppose I'd been working on a program named bleh, that was in directory bleh. I've been working on this program for a long time.

Then one day I do v up and V has added a new bleh command.

Suddenly my build script, my testing, etc., all fail to work any longer, because v bleh now runs the new command instead of building my directory.

I would be very... unamused.

ttytm commented 3 weeks ago

Again, how else would you solve it?

ttytm commented 3 weeks ago

The regression might be more unamusing.

JalonSolov commented 3 weeks ago

It would have been better if V had required the / from the start. Then there would be no problem. Or requiring the build command, such as v build fmt.

As it is, no matter which way you do this, there can be problems. I think it might be nicer to the user to at least warn them of the ambiguity... perhaps tell them the alternatives.

ttytm commented 3 weeks ago

Something like an info message when there is a directory with the same name to target it with the /?

JalonSolov commented 3 weeks ago

Yes. There shouldn't be any surprises. It's ok if the command "wins", but it should also tell you about it instead of silently doing that.

ttytm commented 3 weeks ago

Would that be okay for a separate PR? This one would just fix the regression and keep the behavior as it was.

I think we can agree on improving behavior when there is a file/dir with the same name as a command; it's just that this PR doesn't introduce a new issue. It was the way things were handled before the commands stopped working in the described way.

JalonSolov commented 3 weeks ago

I don't understand... wouldn't it be as simple as adding a notice before the continue in your changes?

If it's more involved than that, then yes, a separate PR would work... as long as it's not forgotten.

ttytm commented 3 weeks ago

Yes, adding it is that simple. Just felt that would be a separate change.

ttytm commented 3 weeks ago

Suggestions on the message are welcome @JalonSolov

ttytm commented 3 weeks ago

Reverting adding the print here as it impacts other tests. I think having it in a dedicated scopes can go with least friction. Then if and what to add, can be handled separately.