Closed marcoesters closed 1 month ago
I was thinking about the setup we use in napari. The installers basically ship:
base
environment with conda and friendsnapari-X.Y.Z
environment, where X.Y.Z is the version of napari.If we merge this PR, napari shortcuts will always have that env name appended, which might be redundant (we will see something like napari (napari-0.4.19)
). I wonder if this is something we should control at the menuinst level or the constructor level. Maybe it should be a special placeholder in the shortcut name instead? So we make it opt-in?
Something like:
...
"name": "Spyder 4.0 {{ ENV_NAME_IF_NOT_BASE }}",
...
Too hacky?
Alternatively, we could add a new field non_base_env_suffix
that could take a template string like (%s)
. But that would require schema changes.
Too hacky?
We are talking with @mrclary to do exactly the same for Spyder! So, if you decide to implement that directly here, we wouldn't need to in our menu.json
I don't think it's too hacky. This is exactly the behavior that many seem to want.
We could alternatively also define the ENV_NAME
that way - that it only renders if it's not the base
environment. However, ENV_NAME_IF_NOT_BASE
would maintain backwards compatibility with other patch versions of version 2.
We are talking with @mrclary to do exactly the same for Spyder! So, if you decide to implement that directly here, we wouldn't need to in our
menu.json
I'm sorry, @ccordoba12, I need to clarify that this is not exactly what we are trying to do with Spyder. What we are doing for Spyder is setting the name of the shortcut as Spyder <major version>
for our installer created (non-base) environment, and Spyder <major version> (<env name>)
for any other conda environment.
I do not support this PR as it currently is because I don't think menuinst
should automatically set any part of the shortcut name, removing flexibility from the developer. This PR would absolutely prevent us from doing what we want to do with Spyder. I would support additional menuinst
placeholders or additional menuinst._schema.MenuItem
s, e.g. in addition to name
, have name_if_base
and/or name_if_not_base
, or similar, with defined precedence.
These would not really do anything for Spyder since menuinst
has no way of knowing whether the environment was created by constructor
, but they would not prevent us from doing it. So we would still use constructor
's pre-install
script in combination with spyder
's post-link
script to distinguish the constructor
made environment and modify the spyder-menu.json
before menuinst
is invoked.
I think a separate menu item is a better way to go about this. A separate placeholder would still require implicit menuinst
logic. For example:
name: App {{ NAME_IF_NOT_BASE }}
would either have to implicitly insert parentheses or would just insert the name without them. Adding an optional name_if_not_base
provides more flexibility.
The more I think about this the less I see a way out, hah. menuinst
is supposed to be not too conda specific. Other package managers could implement hooks to use it with different JSON locations. For example, micromamba
or pixi
could implement the schema and over there there's no concept of base
environment.
I'm not sure I like the idea of adding more items to an implementation agnostic schema, when those items are kind of implementation specific... I need to think more about this 😬
menuinst
does already have the concept of prefix
and base_prefix
though. RIght now, it's only conda
, but utils._default_prefix
could be expanded to accommodate other frameworks.
If they do not have a concept like this, they don't have to use these, i.e., prefix
and base_prefix
will be the same thing.
I'm sorry, @ccordoba12, I need to clarify that this is not exactly what we are trying to do with Spyder
Ok, sorry for the misunderstanding.
I do not support this PR as it currently is because I don't think menuinst should automatically set any part of the shortcut name
You're probably right: any project that depends on menuinst
should be able to decide how to manage to name their shortcuts.
What if we extend name
field into a multi-type field. Right now, it only takes a str
(with placeholders), but we could also allow condition-to-str mappings:
...
"name": {
"prefix_is_base": "Something",
"prefix_is_not_base": "Something (else)"
},
...
I think I like that better, and it's a bit more future proof because if there are more use cases in the future, we won't have to add yet another key, only more conditions to the name
mapping.
What if we extend
name
field into a multi-type field. Right now, it only takes astr
(with placeholders), but we could also allow condition-to-str mappings:... "name": { "prefix_is_base": "Something", "prefix_is_not_base": "Something (else)" }, ...
I think I like that better, and it's a bit more future proof because if there are more use cases in the future, we won't have to add yet another key, only more conditions to the
name
mapping.
@jaimergp, I like this idea. "prefix_is_base" and "prefix_is_not_base" may be a bit confusing, however. "prefix" and "base" may be mis-interpreted as name prefix and base name, rather than referring to the environment prefix and base environment. Perhaps keys such as "default" and "base_env" might be more clear. Users could specify just "default" and have it apply everywhere, but if "base_env" is specified, then that value is applied in the case of a base environment.
What if we extend
name
field into a multi-type field. Right now, it only takes astr
(with placeholders), but we could also allow condition-to-str mappings:... "name": { "prefix_is_base": "Something", "prefix_is_not_base": "Something (else)" }, ...
I think I like that better, and it's a bit more future proof because if there are more use cases in the future, we won't have to add yet another key, only more conditions to the
name
mapping.@jaimergp, I like this idea. "prefix_is_base" and "prefix_is_not_base" may be a bit confusing, however. "prefix" and "base" may be mis-interpreted as name prefix and base name, rather than referring to the environment prefix and base environment. Perhaps keys such as "default" and "base_env" might be more clear. Users could specify just "default" and have it apply everywhere, but if "base_env" is specified, then that value is applied in the case of a base environment.
Sorry for leaving this dormant - just getting back to this after a long leave of absence.
I do like the mapping idea, but we need to make this backwards compatible. Or do we want to have any string be a potential mapping?
I concur with "default" and "base_env" even though I'd probably want to be more explicit and use "base_environment".
I do like the mapping idea, but we need to make this backwards compatible. Or do we want to have any string be a potential mapping?
We should be able to accept a string (original behaviour) or a mapping of conditions to string. Since we have total control over condition keys, we can either get creative with target_environment==base
and target_environment!=base
(but those are not evaluated, just strings we interpret in that way), or using op-less strings like target_environment_is_base
and target_environment_is_not_base
.
I don't care much as long as things are documented nicely.
I do like the mapping idea, but we need to make this backwards compatible. Or do we want to have any string be a potential mapping?
We should be able to accept a string (original behaviour) or a mapping of conditions to string. Since we have total control over condition keys, we can either get creative with
target_environment==base
andtarget_environment!=base
(but those are not evaluated, just strings we interpret in that way), or using op-less strings liketarget_environment_is_base
andtarget_environment_is_not_base
.I don't care much as long as things are documented nicely.
I added the target_environment_is_base
/_not_base
keys and added a test. This should allow for both the new v2 and old v1 behavior to co-exist.
This will need a CEP update.
Description
menuinst v1
added the environment name to shortcuts when the packages are installed outside the base environment. ~This behavior appears to be intended for Windows: https://github.com/conda/menuinst/blob/1df31c6ad71110fb9d037b9d6aca5b35da08d0c3/menuinst/platforms/win.py#L201~~However,
env_name
is never set. This PR setsenv_name
outside the base environment.~Add a feature to re-enable this behavior by expanding the schema. Contrary to v1, this is an option now and not standard behavior to allow for more flexibility.
Checklist - did you ...
news
directory (using the template) for the next release's release notes?