Closed stankudrow closed 4 months ago
@amyreese , good day. Could you have a look at this PR? I think it is a good one to lay a stone for future betterment.
Hm, the documentation gets generated fine locally:
@amyreese , the build docs workflow has been fixed. We can move on to the review, shall we?
Apologies for not responding sooner, was busy traveling for a conference.
I recognize that you put some effort into this PR, and I do appreciate that, but generally speaking, I'm not in favor of it. In the future, it might be better to ask projects up front if this sort of PR would be welcome before spending your time on it.
Poetry is a wide-spread yet powerful tool for managing the project dependencies, project packaging and publishing. This tool is well documented and is being actively maintained.
I'm quite happy with Flit and I like that it supports PEP 621 style metadata rather than needing a tool-specific project config. I also don't particularly like Poetry as a build tool, and don't see the need for something that complex for a library like this.
Beyond that, manually specifying the version in the project metadata is exactly what I don't want to do. I wrote and use the tool attribution
which automates (re)generating the changelog and __version__.py
file, and then creating a matching git tag. This undoes all of that.
The project markdown files were corrected to some extent after having markdown linter complaints addressed.
I don't agree with most markdown linters, and IMO the changes in this case make the files more difficult to read in plain text format. Also the changelog is a generated file and shouldn't be modified by either a linter or a user.
Python 3.6 support dropped in favour of Python 3.9 and higher. The latter was chosen to resolve the installation of the Sphinx package
3.8 is still not EOL. I do not want to drop support for that yet when the package is working just fine otherwise.
For Sphinx specifically, this is why the "dev" and "docs" extras are split — this allows the package to be installed and tested on older Python versions while docs can be built on RTD with a newer version.
The MANIFEST.in file is removed
That is a vestige of when the project used setuptools. Happy to accept a PR just removing that file.
the .gitignore file was enriched
I'd prefer to keep the manually-added html/
directory at the top of the file where it's more obvious to a user. But also the vast majority of these aren't relevant to this project either, so I don't see a need for this.
Is it necessary to have more-itertools package as a main dependency for this project? Since the aioitertools provides the wrappers for the more_itertools, it is reasonable to depend on this package and be obliged to be sync'ed with it
This doesn't wrap more_itertools — it provides an alternative, async implementation. There is no reason for a dependency on that package.
introduce the ruff linter (already included in the pyproject.toml) introduce the pytest and choose necessary plugins (the basic ones are already added in the test group, see the pyproject.toml file)
No thanks. I'm happy with Flake8 and stdlib unittest.
Thank you for a thorough and detailed feedback.
What features could be welcome for this project apart removing the MANIFEST.in
file?
If you find the gitignore updates useful, that would be fine, as long as the html/
ignore stays at the top. If there have been any new functions added to itertools, equivalent async implementations would also be welcome. Otherwise the project is relatively stable aside from any potential for performance improvements or bugfixes, so I don't expect much to change from here.
This Pull Request addresses the following major features concerning the project structure, not the code base (for now):
[x] Project metadata are mostly the same as before (review is required) [x] The MANIFEST.in file is removed (Poetry has the
include
option) [x] Most of markdown files were changed to satisfy markdown linters [x] The Makefile rules were checked and adapted for Poetry [x] the.gitignore
file was enrichedIssues or PRs to cover
170
169
168
166
159
Questions:
more-itertools
package as a main dependency for this project? Since theaioitertools
provides the wrappers for themore_itertools
, it is reasonable to depend on this package and be obliged to be sync'ed with itFuture planes:
ruff
linter (already included in thepyproject.toml
)pytest
and choose necessary plugins (the basic ones are already added in the test group, see thepyproject.toml
file)