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

Restructure and reorganization #53

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

techalchemy
Copy link
Member

  • Separation of concerns - actions are now separated from CLI interface
  • Models are separated from 'internals'
  • May want to think about what to do with internals to organize them
    a bit?
  • Introduced models for most of the Object Oriented stuff
  • Introduced options as a central aggregation of options and
    argument groups - this has 2 classes: Option and ArgumentGroup,
    each with the method add_to_parser for ease of use
  • Command subclasses now simply take a list of options or argument
    groups and have a defined method to override called run(self, options) -- this way we can pass parameters to the corresponding actions
    rather than just blanketing the options in
  • Closes Refactor procedural code in cli mains into operations  #21

Let me know if you have questions or want to change anything here. I tested a bunch of stuff and it seems to work, and (for me anyway) this is starting to make a lot of sense. Happy to work with you to make it clearer, but it's actually a lot easier for me to follow so if it's not too bad for you it'd be super helpful

Here is an example of cli.add:

class Command(BaseCommand):

    name = "add"
    description = "Add packages to project."
    arguments = [package_group]

    def run(self, options):
        if not options.editable_lines and not options.requirement_lines:
            self.parser.error("Must supply either a requirement or --editable")
        return add_packages(
            packages=options.packages,
            editables=options.editables,
            project=options.project,
            dev=options.dev
        )

and the corresponding actions.add

def add_packages(packages=[], editables=[], project=None, dev=False, sync=False, clean=False):
    from passa.models.lockers import PinReuseLocker
    from passa.operations.lock import lock
    ....

This way our core logic isn't tied exclusively to the CLI (and might be accessed by say, pipenv internals someday as a side-benefit)

Here is a new option (from cli.options):

dev_only = Option(
    "--dev", dest="only", action="store_const", const="dev",
    help="only try to modify [dev-packages]",
)

# and an argument group
package_group = ArgumentGroup("packages", options=[packages, editable, dev, no_sync])

- Separation of concerns - actions are now separated from CLI interface
- Models are separated from 'internals'
- May want to think about what to do with internals to organize them
  a bit?
- Introduced `models` for most of the Object Oriented stuff
- Introduced `options` as a central aggregation of options and
  argument groups - this has 2 classes: `Option` and `ArgumentGroup`,
  each with the method `add_to_parser` for ease of use
- Command subclasses now simply take a list of options or argument
  groups and have a defined method to override called `run(self,
  options)` -- this way we can pass parameters to the corresponding `actions`
  rather than just blanketing the options in

Signed-off-by: Dan Ryan <[email protected]>
- Also move `projects` and fix imports

Signed-off-by: Dan Ryan <[email protected]>
Signed-off-by: Dan Ryan <[email protected]>
Signed-off-by: Dan Ryan <[email protected]>
Signed-off-by: Dan Ryan <[email protected]>
@techalchemy
Copy link
Member Author

FYI I am going to add some tests on top of this and merge it if you don't have any issues, since this makes it a lot easier to test as i mentioned somewhere else

- A few minor fixes with the restructure

Signed-off-by: Dan Ryan <[email protected]>
Signed-off-by: Dan Ryan <[email protected]>
@uranusjr
Copy link
Member

Sure, I probably won’t have much time working on this for the next week, and if I find some I’ll work on creating a better mock PyPI server first.

@techalchemy
Copy link
Member Author

No worries I don’t think I changed any functionality just moved stuff around. But if you want to release a patch release of master (or I can if you’re ok with it) I can bundle that as the.backup resolver and cut a release of pipenv

@@ -201,14 +234,18 @@ def sync(self):
class Cleaner(object):
"""Helper class to clean packages not in a project's lock file.
"""
def __init__(self, project, default, develop):
def __init__(self, project, default, develop, sync=True):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does sync do? I am not sure Cleaner needs to be aware of Synchronizer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it for the moment to see how the CLI feels with it (think of it like a --[no-]dry-run option... The actual reason related to a need to short-circuit the test environment's cleaning functionality, so don't take this as me thinking it should stay... just as a kind of 'does this seem like a good idea?' check

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it’s a dry run flag, I’d be more inclined to give it a different name, or (better IMO) to have a DryRunCleaner to do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like a good plan, haven't revisited it yet

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm looking at the code I'm not sure about this actually, I don't see a reason to reimplement the exact same functionality and print the same result just skip one line of activity... this isn't a java project...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants