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

Clean up of explicit namespace declaration #89

Closed
wants to merge 9 commits into from

Conversation

bamsumit
Copy link
Contributor

  • explicit namespace declaration cleanup
  • unittests runnable from root level

@bamsumit bamsumit linked an issue Nov 22, 2021 that may be closed by this pull request
@mathisrichter
Copy link
Contributor

mathisrichter commented Nov 22, 2021

These are the steps @bamsumit and I found to work with PyCharm and the changed init-file convention:

  • put the 'src' directories (e.g., lava/src) of all lava repositories into the PYTHONPATH
  • Mark all 'src' directories (e.g., lava/src) as "Sources Root"
  • Mark all 'tests/lava' (NOT tests!) directories (e.g., lava/tests/lava) as "Test Sources Root"

Copy link
Contributor

@mathisrichter mathisrichter left a comment

Choose a reason for hiding this comment

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

This has both changes for namespace declarations as well as CONV related changes. Is that intended? I guess we should try to avoid mixed PRs like that in the future.
Approving the part about explicit namespaces because I tested this with @bamsumit on my PyCharm setup. Haven't checked the CONV part.

@bamsumit
Copy link
Contributor Author

bamsumit commented Nov 23, 2021

I'll still need to sync this one with main after conv is merged and move the tutorial/ stuff.

@mgkwill
Copy link
Contributor

mgkwill commented Nov 23, 2021

I'll still need to sync this one with main after conv is merged and move the tutorial/ stuff.

As an FYI we should only have content for the 'ONE' thing a PR is about in a PR. It should not include changes for other issues or PRs. Only the changes needed to make the PR work.

I won't hold this back now but in the future I will be somewhat militant about this and make sure changes are not based on other changes or that people separate the issues into two PRs. There are a number of reasons for this, but the main one is maintainability of the project and codebase is harder if we don't have focused commits/PRs. There is some grey area, but including changes for another PR or Issue stands out brightly and should be shunned. Another reason is that it makes things hard to review.

It looks like instead of creating a new branch from main for the changes base, you made the changes on top of #73 branch.

I was going to merge this to help make lava-dnf unit tests pass but we need to look at #73 first, which is another reason not to base changes on other changes. It makes it hard to merge changes separately and then one has a chain of dependencies which will keep changes from being merged or at the least take more time.

@mgkwill
Copy link
Contributor

mgkwill commented Nov 23, 2021

I'll still need to sync this one with main after conv is merged and move the tutorial/ stuff.

As an FYI we should only have content for the 'ONE' thing a PR is about in a PR. It should not include changes for other issues or PRs. Only the changes needed to make the PR work.

I won't hold this back now but in the future I will be somewhat militant about this and make sure changes are not based on other changes or that people separate the issues into two PRs. There are a number of reasons for this, but the main one is maintainability of the project and codebase is harder if we don't have focused commits/PRs. There is some grey area, but including changes for another PR or Issue stands out brightly and should be shunned. Another reason is that it makes things hard to review.

It looks like instead of creating a new branch from main for the changes base, you made the changes on top of #73 branch.

I was going to merge this to help make lava-dnf unit tests pass but we need to look at #73 first, which is another reason not to base changes on other changes. It makes it hard to merge changes separately and then one has a chain of dependencies which will keep changes from being merged or at the least take more time.

P.S. I wanted to say I'm not trying to be harsh or single anyone out. I appreciate the work done here. I'm just trying to build a public body of evidence and a healthy open source hygiene and culture! But I will probably keep putting these missives on anything that falls outside that area.

@bamsumit
Copy link
Contributor Author

I get it @mgkwill. It was an oversight on my end . I'll remove conv dependency and squash the commits.

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.

Remove legacy __init__.py code
3 participants