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

pref: fix regression of command flags not working #21647

Merged
merged 5 commits into from
Jun 10, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Jun 6, 2024

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

JalonSolov commented Jun 6, 2024

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

ttytm commented Jun 6, 2024

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

Requires

v fmt/

Ending slash

@JalonSolov
Copy link
Contributor

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

@ttytm
Copy link
Member Author

ttytm commented Jun 6, 2024

Yep.

How else would you solve that?

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

@JalonSolov
Copy link
Contributor

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

ttytm commented Jun 6, 2024

Again, how else would you solve it?

@ttytm
Copy link
Member Author

ttytm commented Jun 6, 2024

The regression might be more unamusing.

@JalonSolov
Copy link
Contributor

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

ttytm commented Jun 6, 2024

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

@JalonSolov
Copy link
Contributor

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

ttytm commented Jun 6, 2024

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

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

ttytm commented Jun 6, 2024

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

@ttytm
Copy link
Member Author

ttytm commented Jun 6, 2024

Suggestions on the message are welcome @JalonSolov

@ttytm
Copy link
Member Author

ttytm commented Jun 7, 2024

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.

@ttytm ttytm requested a review from spytheman June 7, 2024 11:27
Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would need a test to prevent future regressions, but currently I can not see a good way to do it in a stable and portable way.

Good work.

@spytheman spytheman merged commit 100b3b0 into vlang:master Jun 10, 2024
64 checks passed
@ttytm ttytm deleted the pref/fix-v-commands branch June 10, 2024 18:51
raw-bin pushed a commit to raw-bin/v that referenced this pull request Jul 2, 2024
…subfolder, named after the command, in the current working folder (vlang#21647)
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.

3 participants