Support editing groups with modify commands #110
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 14 commits
March 25, 2019 12:26
Both methods relating to finding all assets within a group have been moved to Utils.
Now that the methods this class needed within MultiNodeCommand have been moved to Utils it made no sense to keep this inheritance. Especially as it wasn't the cleanest way of handling things in the first place
This prevents the command from complaining when the user attempts to modify notes for an entire group. In this case they would not want to specify a singular asset so this adjusment gives the intended behaviour.
This commit also changes the name of the `node` variable to `nodes`. This is mostly so that only one thing needs to be passed with `action`.
Only applies to commands that have no mandatory arguments
Initially extracted the logic for saving the note to each node to make things cleaner but looking at it now I feel it doesn't add much by having this separation. Therefore I have made the decision to remove the method and handle it within the `action` method like it did previously
Make find_all_nodes, find_nodes_in_groups, find_single_nodes & expand_asterisks class methods of the node class & change refrences to them accordingly
Allow it to be called with a string
Have modify notes and modify map inherit from MultiNodeCommand to use the `find_nodes` method
Replace find_nodes with fetch_nodes (locate_nodes is now called find_nodes) and have fetch_nodes return node obejcts This is to soon allow central modification/interaction with the node list
I know later parameters should line up with the first one but that looks awful
DavidMarchant
approved these changes
Mar 25, 2019
There was a problem hiding this comment.
Thanks! I also made did some refactoring the changes of which i will also list here:
- I moved the find_all_nodes, find_nodes_in_groups, find_single_nodes & expand_asterisks methods to be class methods of
Node - had them return node objects instead of paths
- had
modify map&modify notesinherit fromMultiNodeCommandinstead ofSingleNode(as they now by all accounts hit >1 nodes) - I also moved where we make the call to
create_if_non_existenton new assets to be more efficient & to only ask for a type once
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is to address the second half of #93. Both
modify notesandmodify mapnow support the--groupoption. This allows the user to edit data for an entire group from a single editor instance.After a group has been specified the command will behave no differently than usual for the user. But once they have saved and exited from the editor it will iterate through the group and save the details to each asset.
This does overwrite any existing values for the notes or maps respectively so some caution should probably be exercised when using these commands on a group. Currently there is no warning when running these commands with the group flag but if anyone feels it is 100% necessary and wouldn't get in the way then something could be added fairly easily.
Resolves #93 when merged.