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

Add a cli param to specify the search path #1721

Merged
merged 2 commits into from
Nov 28, 2023
Merged

Add a cli param to specify the search path #1721

merged 2 commits into from
Nov 28, 2023

Conversation

jneem
Copy link
Member

@jneem jneem commented Nov 22, 2023

This adds the cli argument version of the NICKEL_PATH environment variable. A couple points that could be bike-shedded:

  • comma-separated vs colon-separated. I feel like comma-separated is "traditional" in arguments while colon-separated is "traditional" in environment variables, but I have no other justification for this choice.
  • the name of the cli arg. Maybe the nickel prefix is unnecessary? I think it's justified for the env var because it lives in a global namespace.

@github-actions github-actions bot temporarily deployed to pull request November 22, 2023 20:12 Inactive
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

The code looks fine. As for the bikeshedding, I would be tempted to say that having a parameter of the same name as the environment variable might avoid confusion (also, just having --path is not really clear; we could have --include-path, but then the two names are really distinct and not even in a prefix relationship).

As for the delimiter, I honestly have no idea, but we should probably pick 5 compilers/interpreters that are both mainstream and known for having a good CLI UX, and research what they do, and why?

@vkleen
Copy link
Contributor

vkleen commented Nov 23, 2023

About the separator, the tradition with compilers is to just pass multiple include-path arguments. For example, gcc -Iinclude -Imore-includes.

@yannham
Copy link
Member

yannham commented Nov 27, 2023

@jneem what do you think about Viktor's answer? Should we make the CLI argument just take multiple values and sidestep the question of the delimiter entirely? At first sight it sounds more appealing/less error-prone than having two different delimiters.

@jneem
Copy link
Member Author

jneem commented Nov 27, 2023

Yes, that sounds like a good plan, I'll try to find some time today to change it.

I also had a quick look at some mainstream compilers (clang, gcc, rustc, ocamlc, ghc, javac). The multiple argument approach does indeed seem the most common (exceptions: ghc supports both multiple arguments and colon-separated arguments; javac supports only colon-separated arguments (I think: I only read the docs, didn't test)).

The naming between env var and cli flag doesn't have a clear precedent. Most compilers seem to only support the short flag -I (or -i, or -L). Exception is javac, with --class-path and --module-path and so on. Support for environment variables seems not so common: rustc, ocamlc, and ghc don't support them. clang and gcc's environment variable names have nothing to do with the cli flag. javac has the same name for environment variable and cli flag.

@yannham
Copy link
Member

yannham commented Nov 27, 2023

Another solution is to rename the env var NICKEL_INCLUDE_PATH (which is somehow more explicit, IMHO) and call the argument --include-path (with short -I) which would be more or less consistent, and be in line with existing tools. Allowing colon-delimited paths probably doesn't hurt (as it's consistent with the environment variable), although it's probably less useful because you can just use the environment variable for that.

@jneem
Copy link
Member Author

jneem commented Nov 27, 2023

While implementing this, it occurred to me that maybe "import" is better than "include", since that's what we call the operator?

@github-actions github-actions bot temporarily deployed to pull request November 27, 2023 21:03 Inactive
@yannham
Copy link
Member

yannham commented Nov 28, 2023

While implementing this, it occurred to me that maybe "import" is better than "include", since that's what we call the operator?

Sounds reasonable, indeed!

@jneem jneem added this pull request to the merge queue Nov 28, 2023
Merged via the queue into master with commit a6dca6a Nov 28, 2023
5 checks passed
@jneem jneem deleted the nickel-path-mk2 branch November 28, 2023 15:14
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