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 the LocalProject #189

Merged
merged 6 commits into from Mar 25, 2019

Conversation

lachmanfrantisek
Copy link
Member

Resolves #178

@lachmanfrantisek lachmanfrantisek changed the title Refactor the LocalProject WIP: Refactor the LocalProject Mar 21, 2019
or self._parse_repo_name_from_git_project()
or self._parse_namespace_from_git_project()
or self._parse_git_url_from_git_repo()
or self._parse_namespace_from_git_repo()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need so many functions? Are we using all of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, not all of them, but I would like to compute as much as we can, so it can be useful in other projects as well.

Copy link
Contributor

@phracek phracek left a comment

Choose a reason for hiding this comment

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

It is read a bit more, but I would like to see, what part is called in such cases.
I don't see a reference from CLI and I would like to see it.
But It is maybe more complicated.

return False

def _parse_namespace_from_git_repo(self):
if self.git_repo and not self.namespace:
Copy link
Contributor

Choose a reason for hiding this comment

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

What about to add there use case when it is called? like LocalProject(git_url, git_service=).
Or maybe even what command use it like propose-update uses two of them.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. If you want to see how the class is being utilized, just open pyčarm and press alt+f7 and that's your most up-to-date answer.

Copy link
Member

Choose a reason for hiding this comment

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

alt+f7 is doing some funny window resizing in my case. What action do you have in mind?

@lachmanfrantisek
Copy link
Member Author

I don't see a reference from CLI and I would like to see it.

Sorry, I do not understand what you want. (This class differs the one defining the CLI parameter type)

@phracek
Copy link
Contributor

phracek commented Mar 21, 2019

LGTM

@lachmanfrantisek
Copy link
Member Author

I would like to improve tests as well. Please, wait for that.

@TomasTomecek
Copy link
Member

Franto, I understand that you want to support as many use cases as possible; what I'd like to see here personally, is to scope the functionality down: look at how we are using it and only support that: to have a few class methods which accept only 2-3 arguments and would be able to return full instance of local_project; WDYT?

packit/local_project.py Outdated Show resolved Hide resolved
- full_name: "$namespace/$repo"
- namespace: namespace of the remote project
- repo_name: name of the remote project
- path_or_url: working_dir if the directory exists
Copy link
Member

Choose a reason for hiding this comment

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

how about we removed this parameter and performed the check in a dedicated function? then we could pass the path or url to LocalProject

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, we need to check this in multiple places. (All CLI functions.)

@lachmanfrantisek
Copy link
Member Author

lachmanfrantisek commented Mar 22, 2019

Franto, I understand that you want to support as many use cases as possible; what I'd like to see here personally, is to scope the functionality down: look at how we are using it and only support that: to have a few class methods which accept only 2-3 arguments and would be able to return full instance of local_project; WDYT?

Current usecases:

  1. We know git_url and need working_dir and (cloned) git_repo.
  2. We know working_dir+git_service and need git_repo+git_project(And also namespace+name)
  3. We know git_url+git_service and need working_dir, (cloned) git_repo and git_project. (And also namespace+name).

For all of them, you can specify branch and there are multiple places, where you do not know if you have git_url or working_dir.

We can probably:

  • remove git_url and working_dir and set only path_or_url. (But why, when Explicit is better than implicit.)
  • remove offline -- it's not used, but can be usefull for dry-run/offline mode ?

I do not think we can remove any other attribute, since we need to calculate it anyway.

I am not sure I got the benefit of multiple class methods.

Personally, I would like to move the class to ogr, where the general sollution makes more sense.

@lachmanfrantisek
Copy link
Member Author

lachmanfrantisek commented Mar 22, 2019

I'll send a little improved version with tests.

edit: I'll send tests later.

@lachmanfrantisek lachmanfrantisek changed the title WIP: Refactor the LocalProject Refactor the LocalProject Mar 25, 2019
@lachmanfrantisek
Copy link
Member Author

lachmanfrantisek commented Mar 25, 2019

Current usecases:
:

I've made a test for each call of LocalProject.

Signed-off-by: Frantisek Lachman <flachman@redhat.com>
Signed-off-by: Frantisek Lachman <flachman@redhat.com>
Signed-off-by: Frantisek Lachman <flachman@redhat.com>
Signed-off-by: Frantisek Lachman <flachman@redhat.com>
Signed-off-by: Frantisek Lachman <flachman@redhat.com>
Signed-off-by: Frantisek Lachman <flachman@redhat.com>
@TomasTomecek TomasTomecek merged commit 6a781d9 into packit:master Mar 25, 2019
@lachmanfrantisek lachmanfrantisek deleted the refactor-local-project branch March 25, 2019 10:12
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.

None yet

4 participants