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

Cleanups and refactors #39

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

Conversation

hseg
Copy link
Contributor

@hseg hseg commented Aug 7, 2024

  • Give main explicit args and add tests -- These are rudimentary for now,
    but at least they check the forbidden/permitted flag combinations indeed work.

  • Refactors for pylint

    • Add/clarify docstrings
    • Remove useless lambdas (lambda x: f(x) is the same as f)
    • Use is None instead of relying on falsiness
    • Use list comprehensions instead of explicit loops
    • Use lazy substitution for logging (pylint W1201)
    • Use explicit encoding on files
    • Inline unnecessary variables
    • Drop unnecessary else branches
    • Drop unnecessary parameters
  • Refactor CompressedFacade -- Use the class structure more

  • Avoid repeated appends -- Use yield to avoid repeated appends

  • Refactor Packager to reflect package splitting -- Instead of duplicating
    variables for the python2 and python branches, just keep a dict of the
    split metadata and iterate over it

hseg added 3 commits August 7, 2024 16:04
I failed to notice f6f0ab8 meant I had not just made PEP517 the default
on python 3, but actually made it the only option.
Add a `--no-pep517` flag to permit overriding this default in the other
direction.

Considered, but rejected:
- Using a `BooleanOptionalAction` to unify the two flags (would bump the
  minimal `argparse` version to 3.8 for marginal gain)
- Keep f6f0ab8's defaulting trick for `IS_PY2` at least -- considering
  we also need to set defaults based on the python version requested,
  it makes more sense to group their setting all together
PEP517 packages still need the build backend as a `makedepend` --
build+install+wheel are insufficient to build the package.
Separate this out from the other `makedepends` to highlight its
importance, and share the configuration across PEP517/non-PEP517 builds.
I didn't notice wenLiangcan#30 unconditionally added space between `depends+=()`
and the packaging lines in `package()`, even if no dependencies were
passed.

Thus, eg for `pip2pkgbuild pip` the result is
```
package() {

    cd "${srcdir}/${_src_folder}"
    python -m installer --destdir="${pkgdir}" dist/*.whl
}
```
where it should be
```
package() {
    cd "${srcdir}/${_src_folder}"
    python -m installer --destdir="${pkgdir}" dist/*.whl
}
```
@hseg hseg marked this pull request as draft September 9, 2024 18:46
@hseg
Copy link
Contributor Author

hseg commented Sep 9, 2024

Somehow, it escaped my notice that my addition of an argv parameter to main broke the script. It should be an easy fix, but I'm a little busy at the moment. I'll try to get this fixed as soon as I can.

We gave main an argv parameter without specifying a default.
This breaks the default invocation by setuptools.
@hseg
Copy link
Contributor Author

hseg commented Sep 12, 2024

Ended up getting quite the scope creep on the refactors, so I'm pushing a version of the branch with just the argv and testing stuff and leaving the refactors for a future PR.

@hseg
Copy link
Contributor Author

hseg commented Sep 12, 2024

Pushed some of my refactors as well, so the edits are less significant (this reworked PR drops the last three refactors in my list above, cf https://github.com/wenLiangcan/pip2pkgbuild/compare/5b39570ba91512f9a0430fede1a5a4f057e95221..e5ddf1c)

@hseg hseg marked this pull request as ready for review September 12, 2024 19:37
@hseg
Copy link
Contributor Author

hseg commented Sep 12, 2024

Sorry, had just a couple trivial edits that I thought might as well get into this PR.

@hseg
Copy link
Contributor Author

hseg commented Sep 15, 2024

(In case anyone was wondering what I meant by scope creep, I'm currently cleaning up the history of my refactors at https://github.com/hseg/pip2pkgbuild/tree/refactor)

- Add/clarify docstrings
- Remove useless lambdas (`lambda x: f(x)` is the same as `f`)
- Use `is None` instead of relying on falsiness
- Use list comprehensions instead of explicit loops
- Use lazy substitution for logging (pylint W1201)
- Use explicit encoding on files
- Inline unnecessary variables
- Drop unnecessary else branches
- Drop unnecessary parameters
- Rename Packager -> Pkgbuild for clarity
- Use __private instead of _private for private methods, cf
  https://docs.python.org/3/reference/expressions.html#index-5
- Remove dead code: _get_source
@hseg
Copy link
Contributor Author

hseg commented Sep 15, 2024

Forgot to actually run the tests, everything should work now.

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