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.

Sign up for GitHub

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

Jump to bottom

poetry refactoring #543

Open
gpongelli opened this issue Apr 4, 2023 · 4 comments · May be fixed by #544
Open

poetry refactoring #543

gpongelli opened this issue Apr 4, 2023 · 4 comments · May be fixed by #544

Comments

@gpongelli
Copy link

gpongelli commented Apr 4, 2023 •
edited
Loading

Hi,
this issue is just to track poetry (and other tools) refactory.

Tools that will be added:

  • Poetry: Manage dependency, build and release
  • Tox: Test your code against environment matrix, lint and artifact check
  • Generate Sphinx docs from tox
  • Testing with Pytest and coverage, from tox env
  • Format with Black and Isort
  • Lint code with Flake8
  • Lint python files with pylint
  • Documentation linter darglint
  • Security oriented linter bandit
  • Misspelling linter codespell
  • Check static type with Mypy (optional)
  • commitizen: Pre-configured version bumping from git commit messages with a single command, using Semantic Versioning
  • Host release changelog into GitHub Pages
  • integration of all the above into GitHub actions, including:
    • publish official release to PyPI automatically when CI success
    • publish documents automatically when CI success
    • extract changelog from CHANGELOG and integrate with release notes automatically

optionally, a command line interface using Click could be added, is it useful ?

For now, the main drawback of adding poetry is that the code must be restructured having a python package that contains modules, instead of the actual all-in-one module.
This led to a MAJOR version bump (0.10.0 -> 1.0.0) because user need to change their import statement from
from requests_html import HTMLSession to from requests_html.session import HTMLSession <- notice the .session that's a new python file into the requests_html package.

@surister let me know your thoughts, if you agree with this I'll proceed.
@gpongelli gpongelli linked a pull request Apr 7, 2023 that will close this issue
Draft
@surister
Copy link
Contributor

surister commented Apr 7, 2023

Hey, sorry for the late respond, just saw your pull request, I see what you are doing but just throwing in 10 tools without proper discussion/usage doesn't bode well with me. Don't get me wrong, I can see myself implementing some of these tools in the near future, but I'd need to properly see/analyze every tool and it'll take some time to approve a PR of this size, it'll actually be faster if the scope is smaller

@gpongelli
Copy link
Author

Hey, sorry for the late respond, just saw your pull request, I see what you are doing but just throwing in 10 tools without proper discussion/usage doesn't bode well with me. Don't get me wrong, I can see myself implementing some of these tools in the near future, but I'd need to properly see/analyze every tool and it'll take some time to approve a PR of this size, it'll actually be faster if the scope is smaller

do you prefer a PR per tool listed above ?

Consider that #544 only adds poetry in place of pipfile & setup.py, the other tools are NOT (and will not be) in that PR.

@aehlke
Copy link

aehlke commented May 23, 2023

this great effort stalled out because the PR is too large?

@gpongelli
Copy link
Author

this great effort stalled out because the PR is too large?

Mostly yes.
On the other side, upgrading to poetry needs also a package structure, instead of the actual single module; this also led to a breaking change for users that need a different import call, that’s why I propose also a MAJOR version bump (that could be easily managed with one tool of the above list).

My PR does only refactor the single module to a packaged structure with separated modules, and this seems to be huge effort to review.

All the tools listed in description are NOT included in this PR, I used to work with them in my packages so, after this PR with poetry will be merged, it became easier to add them to improve whole package quality (and also reduce maintainer stress).

I respect maintainer opinion, so poetry PR will remain in draft until there’s no interest on it from users/maintainers/anyone.

@aehlke thanks for the interest on this discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Poetry gpongelli/requests-html
3 participants
@aehlke @gpongelli @surister