psf / requests-html

Pythonic HTML Parsing for Humans™
http://html.python-requests.org
MIT License
13.72k stars 977 forks source link

poetry refactoring #543

Open gpongelli opened 1 year ago

gpongelli commented 1 year ago

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

Tools that will be added:

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.

surister commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gpongelli commented 1 year ago

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