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

Python #18

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Python #18

wants to merge 21 commits into from

Conversation

MichalGawor
Copy link
Contributor

No description provided.

Copy link
Member

@twagoo twagoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely ready to be merged but I added a couple of small comments. Also as a general comment: the meta guidelines suggest using backticks to format MUST, SHOULD etc.

Use `pip` as default package manage:

* 👉 `MUST` build wheels and install package from the wheel [PEP427](https://peps.python.org/pep-0427/).
* 👉 `MUST` use pytoml + [Poetry](https://python-poetry.org/docs/pyproject/) as SCM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a Python developer. What is SCM in this context? I tried to google it and couldn't find out

* 👉 `MUST` build wheels and install package from the wheel [PEP427](https://peps.python.org/pep-0427/).
* 👉 `MUST` use pytoml + [Poetry](https://python-poetry.org/docs/pyproject/) as SCM
* 👉 `MUST` distribute packages as wheels
* 👉 `MUST` use CLARIN's [wheel builder](https://gitlab.com/CLARIN-ERIC/docker-alpine-wheeler) for building wheels
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope for this review but this project lacks documentation

Comment on lines +72 to +74
[Python's core unittest](https://docs.python.org/3/library/unittest.html)
[Django's unittest extension](https://docs.djangoproject.com/en/4.2/topics/testing/overview/)
[Django-rest-framework unittest extension](https://www.django-rest-framework.org/api-guide/testing/)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are rendered on a single line, add markup to make into a list ?


### XML processing

For `.xml` files processing SHOULD use [lxml](https://lxml.de/).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XML files are not only .xml :)

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.

3 participants