-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor/Simplification changeset #37
Open
sotte
wants to merge
28
commits into
ros-infrastructure:master
Choose a base branch
from
sotte:simplification_changeset
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
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
Still, this should be renamed and refactored later. But now I can use it to catkinize manifest files.
This makes it easier to unittest the extraction. Also the get_fields_from_manifest() method is really simple now. The create_package_xml_str() method can be simplified as well.
Set both to False for now.
- Correct the dependencies that are added - rm replace_parts and conflict_parts for now
and move make_*special*_section methods into on spot.
Is that really it?
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 was issue #27 (which I'll close). Here is the original text.
Catkinize offers this "changeset" or "interactive mode" feature in which it basically prints a big list of all changes. I like the idea but in my experience it is not really too useful. The list is normally too big (especially when you catkinize a stack) and you should verify the changes (probably with git, your favorite diff tool) anyway.
Also the changeset approach obfuscates what really happens. Depending on if file names in the changeset exist (and/or the content) you backup/delete/change files. This is really implicit and, as you know, "explicit is better than implicit" :)
Another point is, that the description of what is going to happen (prompt_changes()) and the logic is divided. The logic itself is also divided into create_changeset() and perform_changes().
So I propose a simplified method outlined below:
(I know it must change a bit for catkinize_stack.)
handle_X return True if it can perform all actions. In dryrun-mode, handle_X does not perform any file operations and therefore is side effect free.
Also the handle_X methods are more flexible that the current changeset-approach.
What do you think?
The pull request is a work in progress!