Fix conditional platforms and logging

[larsoner]

Description

Closes #463
Closes #464

Turns out the issue was a gap in testing and logic: current tests were only "all plugins enabled for the same set of platforms" (which could be all platforms, or a subset of platforms). Once I added a test for "some plugins enabled for not all platforms, some enabled for all platforms" the problem became clear.

I also added logging of the menu and menuitem name in the warnings.

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?

Related PR also opened for conda in conda/conda#16000

[larsoner]

A git grep no-platforms suggests that this existing file was unused, so I added a little test for it.

[marcoesters]

I like the tests, but can we write separate functions for each scenario? That would make individual failures easier to spot.

[lrandersson]

Thanks for your fix @larsoner (Go blue!), I just have some minor comments and I think in general this looks good!

[lrandersson]

Suggested change

	log.warning(f"{self._log_name}: Overwriting terminal profile for {name}.")
	log.warning("%s: Overwriting terminal profile for %s.", self._log_name, name)

I see that it was an f-string before your change but just for consistency with above.

[lrandersson]

I think we need to the same logic also in the code below this part where we currently have:

    if not paths and _maybe_try_user(target_prefix, base_prefix):
        menu, menu_items = _load(metadata_or_path, target_prefix, base_prefix, "user")
        for menu_item in menu_items:
            paths += menu_item.remove()
        paths += menu.remove()

It might not be that easy to test either because I think the tests are not running as an admin and user_is_admin needs to be mocked.

[larsoner]

Ahh I see, I'll at least push the fix

[marcoesters]

Alternatively, instead of adding the check everywhere, we could filter out the menu items at the beginning of the function:

menu, menu_items = _load(metadata_or_path, target_prefix, base_prefix, _mode)
menu_items = [item.enabled_for_platform() for item in menu_items]
if not menu_items:
        warnings.warn(f"Metadata for {menu.name} is not enabled for {sys.platform}")
        return []

[larsoner]

Yeah that's much cleaner!

[marcoesters]

Very nice, thank you for the fix! Just a few minor comments.

[marcoesters]

Alternatively, instead of adding the check everywhere, we could filter out the menu items at the beginning of the function:

menu, menu_items = _load(metadata_or_path, target_prefix, base_prefix, _mode)
menu_items = [item.enabled_for_platform() for item in menu_items]
if not menu_items:
        warnings.warn(f"Metadata for {menu.name} is not enabled for {sys.platform}")
        return []

[marcoesters]

I like the tests, but can we write separate functions for each scenario? That would make individual failures easier to spot.

[larsoner]

Pushed the suggested changes, thanks for the quick reviews @lrandersson @marcoesters !

[marcoesters]

LGTM, assuming the tests pass.