-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[new release] config (0.0.1) #25068
[new release] config (0.0.1) #25068
Conversation
629c8e1
to
8598336
Compare
I'd wait just a bit here, I think we can cram another use-case for this ppx into this release :) |
@leostera if you want to wait, can you mark this PR as draft (and then unmark it when you are done)? Alternatively, if there are no issues, we could merge this and you can make a point release afterwards. What's your preference? |
I find this package name as well pretty confusing. |
8598336
to
3b89dfc
Compare
@raphael-proust thanks for the note, i'll make sure to keep draft PRs for releases on hold 🙏🏼 – it should be good to release once CI is green tho :) @hannesm I'm open to suggestions, I picked |
And I guess also because it's inspired by Rust's conditional compilation with the Alternatively, would |
@yawaramin i'd like to keep this as simple as possible, so limiting it to @raphael-proust hm, CI doesn't even start on this one. Any thoughts? 🤔 |
@leostera sorry, I didn't explain myself clearly. If we assume that people will get confused by the name 'config', then maybe they wouldn't be confused by the name 'if' which can be seen as similar to other compile-time macros like C's module A = A_unix [@@if any(target_os = "macos", target_os = "linux")]
module A = A_win32 [@@if any(target_os = "windows")] And we can explain this as 'it's just a compile-time if'. Then we could also call the package |
Since this is a ppx, I would suggest |
Naming aside, any thoughts on why the CI won't run here? |
CHANGES: ## 0.0.1 Initial release with support for: * Not, Any, and All expressions in the boolean language * Checking for environment variables * Providing `target_os`, `target_arch`, and `target_env` * Support disabling module expressions * Supports disabling entire modules with a single top-level annotation
3b89dfc
to
2e8fbc5
Compare
Okay, it seems that CI is running now. |
It got stuck with one of the large PR and did not restart properly, but I see you retriggered it already :) |
Would everyone be happy if I called this package edit: |
I don't have a strong objection to |
This version includes only the ppx and a few constants defined at load-time for the current target os, arch, and environment (gnu,musl,msvc). But for example, I'd like to use this to map and load a config.toml file to configure the Riot runtime, or specific things on libc.ml. |
It's preferable, I think. On the question of names, you have to separate several things:
There are currently no rules imposed on names, but we should clarify this point and avoid situations like this. The problem is that the name (and the idea)
More concretely, there has already been a naming problem with There are several ways of solving the problem:
That's my two cents. |
Honestly folks, I really think we're approaching this from the wrong angle. The name is not offensive, its not intended to confuse (its not called ocaml-config, or dune-config), and there is working code that is being used somewhere (so there's no obnoxious intentional name squatting). Anything else on top of this feels a little gate-keepey, specially considering there's no clear guidelines for how to name things, just some implicit conventions. In other words, I'd expect my packages to get published if the name is available, and the above criteria passes, in a first-come-first-served basis. But I will recognize that these are my expectations coming from other ecosystems. In any case, I'm waiting on this release to release #25074 and #25201 so I'd love whoever has authority over what goes and doesn't go into opam to just give me a thumbs up or down so I know what to do. |
"name is available on a first-come first-serve basis" I'm fine with such a policy. We have "base", "core", "http", "h2" and somesuch already in opam-repository... (plus I myself (and others) have been pushing for "tls", "x509" etc.) |
I think it still interesting to write down the policy about package names at least. Finally, I don't have strong opinion too on this specific package but the initial thoughts raise a real issue. |
A few things:
I don't have a strong opinion about this (with a caveat, see the bottom of the comment) and I am happy to find together a community consensus. I am happy this PR raised the discussion though, since this policy is among the thing that we should figure out given the fast growth of the opam-repo. For what is worth, I don't have a problem with the In any case, I don't want to take a unilateral decision but want to also hear your opinions (so thanks all for joining the discussion above!) and the opinion of the other maintainers. This will slow down a bit more the merge, I am really sorry about that 🙏 Caveat: if it was purely a ppx package, the standard is to keep |
Given the existence of all these variants, I don't think we shouldn't suddenly put in a barrier to this package. I'm fine with a library called |
Just to be clear, my problem is not really that the name is generic, but I find it confusing that it has the name of an opam subcommand: I agree that we should not block this on a policy discussion when the policy is not there, but I'll wait to see if @leostera is willing to make a name change for this specific package. |
@mseri also relates to the question of opam not having namespaces, I imagine if we had namespaces like eg |
To expand on my thoughts a bit, should we have a policy to not allow package names which conflict with opam subcommand names? Eg maybe we want to avoid The point I am trying to make is that this problem is arising because of a syntactic ambiguity between opam subcommands and package names. Maybe even if we won't have namespaces, we should have some other way to disambiguate package names? |
@mseri LGTM 🚀 |
I fail to see the syntactic ambiguity here, @yawaramin. |
Hi @avsm the issue was pointed out by mseri (that Maybe opam could deprecate unversioned package names in the command line and print out a message telling people to use a versioned package name instead:
|
Ergonomic, lightweight conditional compilation through attributes
CHANGES:
0.0.1
Initial release with support for:
target_os
,target_arch
, andtarget_env