Suggestion: in the plan json, specify the action name for every action, not only for `dyn Action`
Hi,
I opened the plan json in order to understand what the installation is doing and how it's related to the Rust code.
It was quite hard for me to understand, since some actions have a "action" key with the action name, and some don't. After digging, I understand that the tag is provided for dyn Action types and not for concrete types which implement Action. This makes it quite hard to understand what is planned, as you need to go back, perhaps multiple levels, to the parent dyn Action, and then use the code to understand which type of action is actually planned.
Another (smaller) thing which made it a bit harder for me to understand is that the key "action" is used both for a value which is an action, and also for the action name, inside the definition of the action.
I suggest to:
- Rename the "action" key which is the tag to "action_name".
- Provide the "action_name" key for all actions.
I implemented both in https://github.com/DeterminateSystems/nix-installer/pull/1061.
Here's how the diff looks like:
Renaming "action" to "action_name" is trivial (a change in one line), and is the first commit in the PR. I think it is an improvement by itself, so I think it can make sense to make it its own PR.
Adding "action_name" to all actions was more involved. It is quite easy to add to every Action:
#[serde(tag = "action_name", rename = "my_action")]
pub struct MyAction {}
However, this results in having duplicate "action_name" entries in the json for dyn Action actions.
I solved this by making a change to typetag which prevents the duplicate key. The MR currently uses my branch. I hope the typetag maintainers would agree to add this.
What do you think?
Note: I'm now happy with the implementation. It looks nice to my taste, and relies on a change I'm happy with to typetag, so I hope it will be accepted.
https://github.com/DeterminateSystems/nix-installer/pull/1061 addressed this, and will be available in the upcoming 0.21.0 release.