Skip to content
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
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

sotte
Copy link
Contributor

@sotte sotte commented Aug 12, 2013

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

possible = handle_manifest(dryrun=True) and handle_cmake(dryrun=True) and handle_make(dryrun=True)

if possible and confirm('continue?'):
    handle_manifest(dryrun=False)
    handle_cmake(dryrun=False)
    handle_make(dryrun=False)

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!

sotte added 28 commits August 5, 2013 17:18
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.
 - 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant