Repository navigation
Conversation
mihailefter
left a comment
mihailefter
left a comment
There was a problem hiding this comment.
Hi @XLIU-hub! Thanks for the PR. Please find below some suggestions to improve this work further:
- Delete
.travis.ymlsince this is no longer used. - Add
.readthedocs.yamlsince RTD requires this file since 2023; builds without it fail. Check also if the docs compile and make the appropriate changes there. - Build the docs in CI. One step that runs
sphinx-build -W docs docs/_buildwould have caught the_authorserror. - Build and check the package in CI. Adding
python -m buildandtwine check dist/*catches broken metadata or a README that won't render on PyPI. - Update
actions/checkoutandactions/setup-pythonfrom v2 to v7 in the GitHub workflow.
| from re import split | ||
| from typing import Callable | ||
|
|
||
| from .backtranslate import BackTranslate |
There was a problem hiding this comment.
| from .backtranslate import BackTranslate | |
| from .backtranslate import BackTranslate |
It looks like this convenience import was removed. We need to keep it exposed in __init__.py so that external packages can still import it directly instead of having to map out the deep internal path.
| class ProtectedFileType(FileType): | ||
| def __call__(self, string): | ||
| if 'w' in self._mode and exists(string): | ||
| raise IOError('failed to create "{}": file exists.'.format(string)) | ||
| return super(ProtectedFileType, self).__call__(string) |
There was a problem hiding this comment.
I haven't seen this being used anywhere in the code. Is it related to sphinx-argparse but not used there yet?
| @@ -0,0 +1,50 @@ | |||
| [build-system] | |||
| requires = ["setuptools>=61"] | |||
There was a problem hiding this comment.
| requires = ["setuptools>=61"] | |
| requires = ["setuptools>=77"] |
A plain string license = "MIT" needs setuptools ≥ 77. With older versions the build fails with "project.license must be valid exactly by one definition".
| from mutalyzer_backtranslate import _authors, _project, _version, _info | ||
|
|
||
|
|
||
| author = _get_metadata('Author') | ||
| copyright = _get_metadata('Author') | ||
| project = _get_metadata('Name') | ||
| release = _get_metadata('Version') | ||
| author = _authors | ||
| project = _project | ||
| release = _version | ||
| info = _info |
There was a problem hiding this comment.
_authors isn't defined in __init__.py (it's _author).
| from mutalyzer_backtranslate import _authors, _project, _version, _info | |
| author = _get_metadata('Author') | |
| copyright = _get_metadata('Author') | |
| project = _get_metadata('Name') | |
| release = _get_metadata('Version') | |
| author = _authors | |
| project = _project | |
| release = _version | |
| info = _info | |
| from mutalyzer_backtranslate import _author, _project, _version | |
| author = _author | |
| copyright = _author | |
| project = _project | |
| release = _version |
| from argparse import FileType | ||
| from importlib.metadata import PackageNotFoundError, metadata | ||
| from os.path import exists |
There was a problem hiding this comment.
ProtectedFileType isn't used anywhere. Removing it also makes the FileType and exists imports unused.
| "pytest-pep8>=1.0.6", | ||
| "pytest>=4.0.1", |
There was a problem hiding this comment.
pytest-pep8 has some incompatibilities with recent pytest versions. I suggest removing it, together with raising the minimum pytest version.
| "pytest-pep8>=1.0.6", | |
| "pytest>=4.0.1", | |
| "pytest>=8", |
| exclude_lines = ["def main()"] | ||
|
|
||
| [tool.coverage.run] | ||
| source = ["mutalyzer_backtranslate"] No newline at end of file |
There was a problem hiding this comment.
Missing newline at the end of this file.
2 participants
Update packaging
Move packaging metadata into
pyproject.toml, see detailed at #14