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

Move to package structure #88

Merged
merged 6 commits into from
Jan 31, 2021
Merged

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Jan 21, 2021

This PR is based on the discussion in #87
I've split up the code into multiple files. Additionally I refactored the tests to use pytest instead of unittest.

--
Until #87 is merged, this PR contains a commit with those changes. I will remove it once done.

@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #88 (c5ea738) into dev-4.0.0 (8e2c4f1) will increase coverage by 0.04%.
The diff coverage is 98.99%.

Impacted file tree graph

@@              Coverage Diff              @@
##           dev-4.0.0      #88      +/-   ##
=============================================
+ Coverage      98.94%   98.99%   +0.04%     
=============================================
  Files              1        6       +5     
  Lines            380      397      +17     
=============================================
+ Hits             376      393      +17     
  Misses             4        4              
Impacted Files Coverage Δ
piplicenses/parse.py 94.91% <94.91%> (ø)
piplicenses/table.py 98.93% <98.93%> (ø)
piplicenses/__main__.py 100.00% <100.00%> (ø)
piplicenses/argparse.py 100.00% <100.00%> (ø)
piplicenses/const.py 100.00% <100.00%> (ø)
piplicenses/utils.py 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e2c4f1...c5ea738. Read the comment docs.

@cdce8p cdce8p force-pushed the package-structure branch 3 times, most recently from 34636db to e3478e6 Compare January 22, 2021 14:33
@raimon49
Copy link
Owner

I've prepared the following remote branch.
https://github.com/raimon49/pip-licenses/tree/dev-4.0.0

It would be good to consolidate the results of this P-R into a dev-4.0.0 branch.

@cdce8p cdce8p changed the base branch from master to dev-4.0.0 January 23, 2021 09:50
@cdce8p cdce8p force-pushed the package-structure branch 2 times, most recently from 5f30e8a to a53307e Compare January 23, 2021 09:57
@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 23, 2021

I rebased the branch onto dev-4.0.0, so it's ready for review.

I will however leave it in Draft status until the issue with the squash commit is resolved: #87 (comment)
That will only require a rebase, so the code itself won't change.

@cdce8p cdce8p force-pushed the package-structure branch from a53307e to b281f11 Compare January 23, 2021 11:31
@cdce8p cdce8p marked this pull request as ready for review January 23, 2021 11:33
@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 23, 2021

I've rebased the branch. The PR is ready for review now

@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 28, 2021

@raimon49 Did you get a chance to take a look at this yet?

@raimon49
Copy link
Owner

@cdce8p Overall, I am willing to distribute pip-licenses under the MIT license.

So, I would like all piplicenses/*.py files to have a comment with the license clause.

@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 29, 2021

@cdce8p Overall, I am willing to distribute pip-licenses under the MIT license.

So, I would like all piplicenses/*.py files to have a comment with the license clause.

I agree, the MIT license is a really good fit for this project. However I don't believe it's really necessary to add it to every file header. In almost all cases it's enough to have a LICENSE file distributed with the package. Doing it that way, it's also way easier to update the copyright notice (eg. to update the year). The only cases I can think of including the license in every file use the GPL License and even then I personally would advise against it.

Copy link
Owner

@raimon49 raimon49 left a comment

Choose a reason for hiding this comment

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

The split design of the package structure looks like a good job.

After this PR is merged, or just before v-4.0.0 ships, I may add a simple link to the license file URLs and a declaration of character encoding to all files.

@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 31, 2021

After this PR is merged, or just before v-4.0.0 ships, I may add a simple link to the license file URLs and a declaration of character encoding to all files.

I don't think it's necessary, but I can understand your intension. I've added a short note to the top of every source file:

# ---------------------------------------------------------------------------
# Licensed under the MIT License. See LICENSE file for license information.
# ---------------------------------------------------------------------------

What do you think?

--

Character encodings: I believe they aren't necessary. utf-8 is the default in Python 3. See stackoverflow. Vim doesn't need it either.

--

#!/usr/bin/env python: Also not necessary, since the python files aren't executed directly.

@cdce8p cdce8p force-pushed the package-structure branch from e56bb17 to c5ea738 Compare January 31, 2021 12:21
@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 31, 2021

@raimon49 Can you squash merge this PR (and all other subsequent ones for the dev branch) as well?

@raimon49
Copy link
Owner

@cdce8p

I've added a short note to the top of every source file:

Thanks for the work. It looks good.

Character encodings: I believe they aren't necessary. utf-8 is the default in Python 3. See stackoverflow. Vim doesn't need it either.

Okay, I understand.

@raimon49
Copy link
Owner

Can you squash merge this PR (and all other subsequent ones for the dev branch) as well?

Yes, we will adopt that workflow for the merge to dev-4.0.0.

@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 31, 2021

Can you squash merge this PR (and all other subsequent ones for the dev branch) as well?

Yes, we will adopt that workflow for the merge to dev-4.0.0.

Thanks 👍🏻

@raimon49 raimon49 merged commit de07741 into raimon49:dev-4.0.0 Jan 31, 2021
@cdce8p cdce8p deleted the package-structure branch February 2, 2021 11:51
raimon49 pushed a commit that referenced this pull request Feb 3, 2021
* Move to package structure

* Copy paste
* Fixed imports
* Test file only moved!

* Fixed tests

* Refactor tests + move to pytest

* Add short license headers

* Enable github actions on dev-4.0.0 branch

* Remove license header from setup.py
raimon49 pushed a commit that referenced this pull request Feb 20, 2021
* Move to package structure

* Copy paste
* Fixed imports
* Test file only moved!

* Fixed tests

* Refactor tests + move to pytest

* Add short license headers

* Enable github actions on dev-4.0.0 branch

* Remove license header from setup.py
raimon49 pushed a commit that referenced this pull request Feb 28, 2021
* Move to package structure

* Copy paste
* Fixed imports
* Test file only moved!

* Fixed tests

* Refactor tests + move to pytest

* Add short license headers

* Enable github actions on dev-4.0.0 branch

* Remove license header from setup.py
raimon49 pushed a commit that referenced this pull request May 3, 2021
* Move to package structure

* Copy paste
* Fixed imports
* Test file only moved!

* Fixed tests

* Refactor tests + move to pytest

* Add short license headers

* Enable github actions on dev-4.0.0 branch

* Remove license header from setup.py
raimon49 pushed a commit that referenced this pull request May 3, 2021
* Move to package structure

* Copy paste
* Fixed imports
* Test file only moved!

* Fixed tests

* Refactor tests + move to pytest

* Add short license headers

* Enable github actions on dev-4.0.0 branch

* Remove license header from setup.py
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.

2 participants