New build system #243
Merged
New build system #243
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This "conversion" process now has two levels: The low-level part is in `valuetype`, where ics value strings are converted to simple python objects and back, based on some externally determined VALUE type and without using any context or inspecting parameters. The high-level part is in `converter`. A `GenericConverter` can take multiple ContentLines or Containers and convert them into the respective values of a `Component`. For `AttributeConverter`s this is done for a single attribute of a `Component` type (which must be an `attr`s class), using metadata of the attribute to determine further information, such as value type, is required or multi value, ics name, etc. `AttributeValueConverter`s then use a `valuetype` converter to parse the simple attribute value. Other converters combine multiple `ContentLine`s into a single attribute, e.g. a `Timespan`, `Person`, or `rrule`. The `ComponentConverter`, which is created from the `Meta` attribute set on any `Component` subclass, inspects all attributes of the class and calls the respective converters. All unknown parameters are now also collected in a dict. This makes it a lot less work to add new attributes, as most conversion logic can be generated automatically and without any redundant code. Additionally, this makes it easier to implement correct handling of `VTIMEZONE`s in all places and will allow implementation of JSON-based ical handling and variable levels of parser strictness. I also included some further refactorings: the `tools` module with the broken online validation is gone (the website is offline), as all `Alarm`s now only take a few lines they have been merged into a single file, `ics.grammar.parse` has been shortened to `ics.grammar`, the inner `Meta` classes has been replaced by instances of an `attr`s class, the `Component` conversion methods are now called `from_container` and `to_container` and for `ContentLine`/`Container` there's now a `serialize` method to convert them to ics strings. The most important change might be that all custom `__str__` and `__repr__` methods were removed. They now default to what `attr` generates and in general follow the standard that `repr`s "should look like a valid Python expression that could be used to recreate an object with the same value". This makes debugging easier and allows us to implement `str` with a nice and short informal representation, and only generate the ics representation when this is intended. Previously, `__str__` returned the ics string and `__repr__` returned a very short informal description, which made dumping the actual python values hard when debugging.
ValueConverters are now allowed to modify optional params and context, i.e. consume params and store context when parsing, and add params when serializing. also move ExtraParams to types, use NewType instead of a direct alias to catch invalid dict usage, and ensure that they are copied using deep-copy (they might contain lists), add EmptyDict as argument default, fix timespan context clean-up
this was done using `bumpversion --verbose release`
this was done using `bumpversion --verbose minor`
this was done using `bumpversion --verbose release`
…st tests/) to fix PATH problems
`noqa` and `type: ignore` are now only used for actual bugs in the checkers unfortunately, current pyflakes dislikes `type: ignore[something]`, so we can't ignore specific mypy bugs until pyflakes 2.2 is in flakes8
Closed
tomschr
suggested changes
Apr 20, 2020
CONTRIBUTING.rst
Outdated
Comment on lines
169
to
172
| Please note that bumpversion directly makes a commit with the new version if you don't | ||
| pass ``--no-commit`` or ``--dry-run``, | ||
| but that's no problem as you can easily amend any changes you want to make. | ||
| Further things to check: |
There was a problem hiding this comment.
I'm not sure about the "but that's no problem". You mean probably to amend an existing commit with git commit --amend, right? Hmn, I would avoid that information. A commit done by bumpversion should be not diluted with something else.
Suggested change
| Please note that bumpversion directly makes a commit with the new version if you don't | |
| pass ``--no-commit`` or ``--dry-run``, | |
| but that's no problem as you can easily amend any changes you want to make. | |
| Further things to check: | |
| Note, that bumpversion directly makes a commit when bumping a new version. If you | |
| want to avoid this, add the option ``--no-commit`` or ``--dry-run`` to the command. | |
| Further things to check: |
There was a problem hiding this comment.
Yeah, git commit --amend was what I was thinking about, as I thought all release-related information being collected in a single commit would be nice. If that's not what we want, we should probably also change the vi CHANGELOG.rst && git commit -i CHANGELOG.rst --amend bash code / steps above.
Co-Authored-By: Tom Schraitle <tomschr@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## new-parser-impl #243 +/- ##
================================================
Coverage 66.06% 66.06%
================================================
Files 23 23
Lines 2104 2104
================================================
Hits 1390 1390
Misses 714 714 Continue to review full report at Codecov.
|
@tomschr thanks a lot for checking that! This PR is all about infra and having a nicely coded project won't help anyone else if the documentation is no use. :) Regarding the actual source code changes in this PR, most of them are there because I made these changes against my local version and not a fresh clone, so that's mostly small stuff that doesn't quite belong here, but which I would have to commited to the other branch anyways. They're about checker happyness and contain no functional changes, so it's probably not worth sieving them out manually, as the commits will get squashed in the end. |
|
@C4ptainCrunch, @aureooms do you have an opinion on this? This is currently somewhat blocking my work on 0.8 and it would be nice if I could merge this and continue working on the parser next weekend or so. |
@N-Coder Looks fines to me. Great work! |
Mostly by moving/splitting test dependencies to different sections in tox.ini as mypy and pypy don't work well together and it is sufficient to run mypy checks on CPython.
N-Coder
added a commit
that referenced
this pull request
May 16, 2020
For my current PR #240 I'm adding `hypothesis` as a test dependency to allow generation of random input strings to better test parsing and escaping logic (see e44e4b3). I also changed how we load the grammar file, to make the package zip safe again (see 3cafe53). While working on how these could be integrated into our testsuite, I found out that `setup.py test` is deprecated ([1](pytest-dev/pytest#5534) [2](https://tox.readthedocs.io/en/latest/example/basic.html#integration-with-setup-py-test-command)). The recommendation was to use `tox` to test the generated package directly using `pytest` instead of relying on `setup.py` to test the sources. Additionally, there's a designated successor to `setup.py` and `setup.cfg` (in which we probably accumulated a few outdated definitions), the build-system independent `pyproject.toml`. Additionally, there are newer tools like `pipenv`, `flit` and `poetry` to help with the whole build and publishing process. As I was not quite happy with how the whole development process of ics.py was set up, I wanted to give them a shot. Collecting some opinion around the internet, it seemed that `flit` was mostly targeted at very simple low-configuration projects and the development of `pipenv` somehow stagnated, while `poetry` seemed to be a well suited solution. So this PR contains my attempt at migrating to `poetry`, with all ics.py sources moved according to the new recommended format. It's mostly the config files in the root directory that changed, but I also removed the `dev` directory as it should no longer be needed. Next to all files from the `./ics/` directory remain unchanged and are simply moved to `./src/ics/`. I didn't copy the tests over yet, as I plan to rewrite most of them in my other branch. The first of the two main configuration files is now `pyproject.toml`, where all meta-information on the project and how it can be installed (i.e. dependencies) and built are stored (without the need to actually execute the file and have some specific setuptools lying around). The second is `tox.ini`, where all testing related functionality is configured. A third file `.bumpversion.cfg` is from a small tool that helps with updating the version number in all places when doing a new release. The `poetry.lock` file optionally stores the dependency versions against which we want to develop, which is independent from the versions the library pulls in as dependency itself, where we are pretty liberal, and the versions we test against, which is always the latest releases. All library sourcecode now resides within a `src` folder, which is recommended as it prevents you from accidentally having the sources in your PATH when you want to test the final package. The root directory now looks very clean and all those files have their specific purpose. If you want to configure how testing is done, you find all information in [`tox.ini`](https://github.com/N-Coder/ics.py-poetry/blob/master/tox.ini). If you want to run the tests (i.e. pytest, doctest, flake8, mypy and check that the docs build), simply run `tox` - you don't have to worry about which versions in which venvs are installed and whether you're directly testing against the sources or against a built package, tox handles all that for you. This not only comes in very handy when running the tests manually, but should also ensure that [CI](https://github.com/N-Coder/ics.py-poetry/blob/master/.github/workflows/pythonpackage.yml) does exactly the same. On a side note, we're now again publishing [coverage data](https://codecov.io/gh/N-Coder/ics.py-poetry). If you just want to run the tests and don't need to fiddle around with the development version of ics in an interactive shell, that's all you need. For the fiddling part, just run [`poetry install`](https://python-poetry.org/docs/cli/#install) and you will have a turnkey development environment set up. Use `poetry shell` or `poetry run` to easily access the venv poetry set up for you. Publishing is now also very simple: just call `poetry publish --build` and the tool will take care of the rest. This made it very easy to make some releases on the [testing pypi instance](https://test.pypi.org/project/ics/#history). The third and last tool you might want is `bumpversion`, if you are making new releases. But there is no need anymore to handle any venvs yourself or to install all ics.py dependencies globally. To summarize, if you want to hit the ground running and publish a new release on a newly set-up machine, the following should suffice: ```bash git clone https://github.com/N-Coder/ics.py-poetry.git && cd ics.py-poetry pip install tox poetry bumpversion --user tox # make sure all the test run bumpversion --verbose release # 0.8.0-dev -> 0.8.0 (release) poetry build # build the package tox --recreate # ensure that the version numbers are consistent # check changelog and amend if necessary git push && git push --tags poetry publish # publish to pypi bumpversion --verbose minor # 0.8.0 (release) -> 0.9.0-dev git push && git push --tags ``` You can try that out if you want -- except for the publishing part maybe. Also note that `bumpversion` directly makes a commit with the new version if you don't pass `--no-commit` or `--dry-run`, but that's no problem as you can easily amend any changes you want to make, e.g. to the changelog. The above information on developing, testing and publishing can now also be found in the docs (see CONTRIBUTING.rst). As these changes are partially based upon #240 but are also quite fundamental, I wanted to collect feedback first before including the changes into #240. The only other thing #240 is still lacking is more testing (only few files already have close to 100% coverage), and I'd prefer to provide that using `tox` in this new environment. So that's also some kind of cyclic dependency. Sorry for the (now superfluous) issue I opened before. So @C4ptainCrunch (and maybe also @aureooms and @tomschr), what's your opinion on this? * migrate repo structure to poetry * fix src path for pytest * add doc skeleton * implement handling of attachments * import project files * set version * fix sphinx build with poetry * don't use poetry within tox see python-poetry/poetry#1941 (comment) * fix timezone tests * change coveralls action * try codecov * bugfixes * add bumpversion * separate src inspection (flake8+mypy src/) from package testing (pytest tests/) to fix PATH problems * bugfixes * Merge branch 'master' into new-parser-impl * remove old files * add dev and publish instructions * checker happiness `noqa` and `type: ignore` are now only used for actual bugs in the checkers unfortunately, current pyflakes dislikes `type: ignore[something]`, so we can't ignore specific mypy bugs until pyflakes 2.2 is in flakes8 * more checker happiness * Apply suggestions from code review Co-Authored-By: Tom Schraitle <tomschr@users.noreply.github.com> * use gitignore directly from github instead of gitignore.io * Apply suggestions from code review to tox.ini * fix tox.ini * add pypy support Mostly by moving/splitting test dependencies to different sections in tox.ini as mypy and pypy don't work well together and it is sufficient to run mypy checks on CPython. * update developing documentation * fix non-ASCII whitespace handling * update test/dev dependencies
N-Coder
added a commit
that referenced
this pull request
May 16, 2020
For my current PR #240 I'm adding `hypothesis` as a test dependency to allow generation of random input strings to better test parsing and escaping logic (see e44e4b3). I also changed how we load the grammar file, to make the package zip safe again (see 3cafe53). While working on how these could be integrated into our testsuite, I found out that `setup.py test` is deprecated ([1](pytest-dev/pytest#5534) [2](https://tox.readthedocs.io/en/latest/example/basic.html#integration-with-setup-py-test-command)). The recommendation was to use `tox` to test the generated package directly using `pytest` instead of relying on `setup.py` to test the sources. Additionally, there's a designated successor to `setup.py` and `setup.cfg` (in which we probably accumulated a few outdated definitions), the build-system independent `pyproject.toml`. Additionally, there are newer tools like `pipenv`, `flit` and `poetry` to help with the whole build and publishing process. As I was not quite happy with how the whole development process of ics.py was set up, I wanted to give them a shot. Collecting some opinion around the internet, it seemed that `flit` was mostly targeted at very simple low-configuration projects and the development of `pipenv` somehow stagnated, while `poetry` seemed to be a well suited solution. So this PR contains my attempt at migrating to `poetry`, with all ics.py sources moved according to the new recommended format. It's mostly the config files in the root directory that changed, but I also removed the `dev` directory as it should no longer be needed. Next to all files from the `./ics/` directory remain unchanged and are simply moved to `./src/ics/`. I didn't copy the tests over yet, as I plan to rewrite most of them in my other branch. The first of the two main configuration files is now `pyproject.toml`, where all meta-information on the project and how it can be installed (i.e. dependencies) and built are stored (without the need to actually execute the file and have some specific setuptools lying around). The second is `tox.ini`, where all testing related functionality is configured. A third file `.bumpversion.cfg` is from a small tool that helps with updating the version number in all places when doing a new release. The `poetry.lock` file optionally stores the dependency versions against which we want to develop, which is independent from the versions the library pulls in as dependency itself, where we are pretty liberal, and the versions we test against, which is always the latest releases. All library sourcecode now resides within a `src` folder, which is recommended as it prevents you from accidentally having the sources in your PATH when you want to test the final package. The root directory now looks very clean and all those files have their specific purpose. If you want to configure how testing is done, you find all information in [`tox.ini`](https://github.com/N-Coder/ics.py-poetry/blob/master/tox.ini). If you want to run the tests (i.e. pytest, doctest, flake8, mypy and check that the docs build), simply run `tox` - you don't have to worry about which versions in which venvs are installed and whether you're directly testing against the sources or against a built package, tox handles all that for you. This not only comes in very handy when running the tests manually, but should also ensure that [CI](https://github.com/N-Coder/ics.py-poetry/blob/master/.github/workflows/pythonpackage.yml) does exactly the same. On a side note, we're now again publishing [coverage data](https://codecov.io/gh/N-Coder/ics.py-poetry). If you just want to run the tests and don't need to fiddle around with the development version of ics in an interactive shell, that's all you need. For the fiddling part, just run [`poetry install`](https://python-poetry.org/docs/cli/#install) and you will have a turnkey development environment set up. Use `poetry shell` or `poetry run` to easily access the venv poetry set up for you. Publishing is now also very simple: just call `poetry publish --build` and the tool will take care of the rest. This made it very easy to make some releases on the [testing pypi instance](https://test.pypi.org/project/ics/#history). The third and last tool you might want is `bumpversion`, if you are making new releases. But there is no need anymore to handle any venvs yourself or to install all ics.py dependencies globally. To summarize, if you want to hit the ground running and publish a new release on a newly set-up machine, the following should suffice: ```bash git clone https://github.com/N-Coder/ics.py-poetry.git && cd ics.py-poetry pip install tox poetry bumpversion --user tox # make sure all the test run bumpversion --verbose release # 0.8.0-dev -> 0.8.0 (release) poetry build # build the package tox --recreate # ensure that the version numbers are consistent # check changelog and amend if necessary git push && git push --tags poetry publish # publish to pypi bumpversion --verbose minor # 0.8.0 (release) -> 0.9.0-dev git push && git push --tags ``` You can try that out if you want -- except for the publishing part maybe. Also note that `bumpversion` directly makes a commit with the new version if you don't pass `--no-commit` or `--dry-run`, but that's no problem as you can easily amend any changes you want to make, e.g. to the changelog. The above information on developing, testing and publishing can now also be found in the docs (see CONTRIBUTING.rst). As these changes are partially based upon #240 but are also quite fundamental, I wanted to collect feedback first before including the changes into #240. The only other thing #240 is still lacking is more testing (only few files already have close to 100% coverage), and I'd prefer to provide that using `tox` in this new environment. So that's also some kind of cyclic dependency. Sorry for the (now superfluous) issue I opened before. So @C4ptainCrunch (and maybe also @aureooms and @tomschr), what's your opinion on this? * migrate repo structure to poetry * fix src path for pytest * add doc skeleton * implement handling of attachments * import project files * set version * fix sphinx build with poetry * don't use poetry within tox see python-poetry/poetry#1941 (comment) * fix timezone tests * change coveralls action * try codecov * bugfixes * add bumpversion * separate src inspection (flake8+mypy src/) from package testing (pytest tests/) to fix PATH problems * bugfixes * Merge branch 'master' into new-parser-impl * remove old files * add dev and publish instructions * checker happiness `noqa` and `type: ignore` are now only used for actual bugs in the checkers unfortunately, current pyflakes dislikes `type: ignore[something]`, so we can't ignore specific mypy bugs until pyflakes 2.2 is in flakes8 * more checker happiness * Apply suggestions from code review Co-Authored-By: Tom Schraitle <tomschr@users.noreply.github.com> * use gitignore directly from github instead of gitignore.io * Apply suggestions from code review to tox.ini * fix tox.ini * add pypy support Mostly by moving/splitting test dependencies to different sections in tox.ini as mypy and pypy don't work well together and it is sufficient to run mypy checks on CPython. * update developing documentation * fix non-ASCII whitespace handling * update test/dev dependencies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For my current PR #240 I'm adding
hypothesisas a test dependency to allow generation of random input strings to better test parsing and escaping logic (see e44e4b3). I also changed how we load the grammar file, to make the package zip safe again (see 3cafe53). While working on how these could be integrated into our testsuite, I found out thatsetup.py testis deprecated (1 2). The recommendation was to usetoxto test the generated package directly usingpytestinstead of relying onsetup.pyto test the sources. Additionally, there's a designated successor tosetup.pyandsetup.cfg(in which we probably accumulated a few outdated definitions), the build-system independentpyproject.toml. Additionally, there are newer tools likepipenv,flitandpoetryto help with the whole build and publishing process. As I was not quite happy with how the whole development process of ics.py was set up, I wanted to give them a shot. Collecting some opinion around the internet, it seemed thatflitwas mostly targeted at very simple low-configuration projects and the development ofpipenvsomehow stagnated, whilepoetryseemed to be a well suited solution.So this PR contains my attempt at migrating to
poetry, with all ics.py sources moved according to the new recommended format.It's mostly the config files in the root directory that changed, but I also removed the
devdirectory as it should no longer be needed. Next to all files from the./ics/directory remain unchanged and are simply moved to./src/ics/. I didn't copy the tests over yet, as I plan to rewrite most of them in my other branch.The first of the two main configuration files is now
pyproject.toml, where all meta-information on the project and how it can be installed (i.e. dependencies) and built are stored (without the need to actually execute the file and have some specific setuptools lying around). The second istox.ini, where all testing related functionality is configured. A third file.bumpversion.cfgis from a small tool that helps with updating the version number in all places when doing a new release. Thepoetry.lockfile optionally stores the dependency versions against which we want to develop, which is independent from the versions the library pulls in as dependency itself, where we are pretty liberal, and the versions we test against, which is always the latest releases. All library sourcecode now resides within asrcfolder, which is recommended as it prevents you from accidentally having the sources in your PATH when you want to test the final package.The root directory now looks very clean and all those files have their specific purpose. If you want to configure how testing is done, you find all information in
tox.ini. If you want to run the tests (i.e. pytest, doctest, flake8, mypy and check that the docs build), simply runtox- you don't have to worry about which versions in which venvs are installed and whether you're directly testing against the sources or against a built package, tox handles all that for you. This not only comes in very handy when running the tests manually, but should also ensure that CI does exactly the same. On a side note, we're now again publishing coverage data.If you just want to run the tests and don't need to fiddle around with the development version of ics in an interactive shell, that's all you need. For the fiddling part, just run
poetry installand you will have a turnkey development environment set up. Usepoetry shellorpoetry runto easily access the venv poetry set up for you. Publishing is now also very simple: just callpoetry publish --buildand the tool will take care of the rest. This made it very easy to make some releases on the testing pypi instance.The third and last tool you might want is
bumpversion, if you are making new releases. But there is no need anymore to handle any venvs yourself or to install all ics.py dependencies globally. To summarize, if you want to hit the ground running and publish a new release on a newly set-up machine, the following should suffice:You can try that out if you want -- except for the publishing part maybe. Also note that
bumpversiondirectly makes a commit with the new version if you don't pass--no-commitor--dry-run, but that's no problem as you can easily amend any changes you want to make, e.g. to the changelog.The above information on developing, testing and publishing can now also be found in the docs (see CONTRIBUTING.rst). As these changes are partially based upon #240 but are also quite fundamental, I wanted to collect feedback first before including the changes into #240. The only other thing #240 is still lacking is more testing (only few files already have close to 100% coverage), and I'd prefer to provide that using
toxin this new environment. So that's also some kind of cyclic dependency.Sorry for the (now superfluous) issue I opened before. So @C4ptainCrunch (and maybe also @aureooms and @tomschr), what's your opinion on this?