Support different names for base and non-base environment

[marcoesters]

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:

menuinst/menuinst/platforms/win.py

Line 201 in 1df31c6

env_suffix = f" ({self.menu.env_name})" if self.menu.env_name else ""

However, env_name is never set. This PR sets env_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 ...

• Add a file to the news directory (using the template) for the next release's release notes?
• Add / update necessary tests?
• Add / update outdated documentation?

[jaimergp]

I was thinking about the setup we use in napari. The installers basically ship:

• A base environment with conda and friends
• A napari-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.

[ccordoba12]

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

[marcoesters]

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.

[mrclary]

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.MenuItems, 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.

[marcoesters]

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.

[jaimergp]

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 😬

[marcoesters]

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.

[ccordoba12]

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.

[jaimergp]

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.

[mrclary]

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.

@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.

[marcoesters]

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.

@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".

[jaimergp]

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.

[jaimergp]

At this point we have not rendered the {{ variables }}, right?

[marcoesters]

That's correct.

The tests for a dictionary representation of the name would have failed by then, so even if code changes render the name at that point, the tests will catch that.

[jaimergp]

Can this be false-y? 🤔 I think we don't need the or "" part, right?

[marcoesters]

Now that I look at it again, I don't think so. I guess I was overly cautious here.

[jaimergp]

This will need a CEP update.