Fixed multiple issues related to blocks and the lmove command
#506
Conversation
This file contains hidden or 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
- Added `--keep-page-size` option to `grid` command - API: removed `temp_document()` context manager from `vpype_cli.State()` as it was a faulty construct - API: added equality operator to `vpype.LineCollection` and `vpype.Document` (useful for writing tests) - Fixed issue where certain nested commands could lead totally unexpected results with blocks - Fixed issue with the `lmove` command where order would not be respected in certain case (such as `lmove all 2`, content of layer 2 wasn't placed after content of layer 1) - Fixed issue where expressions could not use some variable name corresponding to units (e.g. `m`). Expressions may now reuse these variable name (but that would obviously affect their subsequent use). - Improved implementation of `vpype_cli.execute()` - Test: added `data/test_vpy` directory where all .vpy are executed and checked against reference SVGs (using the `reference_svg` fixture). - Tests misc: * added set_current_directory context manager * added make_line_collection factory fixture * added make_document factory fixture * moved block-related tests to `test_block.py` * expanded block-related tests
- Added generation of error report when `reference_svg` fixture fails - Improved test case naming - Updated ref SVGs - Updated CI to save artifacts
This makes the tests more predictable.
Codecov Report
@@ Coverage Diff @@
## master #506 +/- ##
==========================================
+ Coverage 94.44% 94.69% +0.24%
==========================================
Files 61 63 +2
Lines 5457 5636 +179
Branches 795 810 +15
==========================================
+ Hits 5154 5337 +183
+ Misses 185 182 -3
+ Partials 118 117 -1
Continue to review full report at Codecov.
|
|
Kudos, SonarCloud Quality Gate passed!
|
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.
Description
This PR aims to address multiple issues which arise from the discussion in #487, and in particular the "paint well" command listed in #501.
The first issue is a bug in blocks which, due to the structurally faulty
vpype_cli.State.temp_document()context manager, could disregard the outcome of the nested commands if one such command changed the pipeline'svp.Documentinstance. This is solved by suppressing said context manager and rewriting the block commands' code.A new
--keep-page-sizeoption has been added to thegridcommand in the process (helps for tests).Another issue is the order of the
lmovecommand when used likelmove all 2. Previously, the resulting ordering would be: [layer 2 content, layer 1 content, layer 3 content, etc.]. This would break the intended "paint well" pipeline. This PR fixeslmovesuch that the order is instead [layer 1 content, layer 2 content, layer 3 content, ...].Finally, another minor issue was the read-only nature of the unit variable in expressions, in particular the newly introduced
m. This meant that amvariable couldn't be created, leading to a confusing message. These variables are now no longer marked as read-only, and can be overwritten by expressions (with the caveat that this would, of course, break their functionality).To address these issues and changes, multiple tests have been added, as well as the scaffolding to automatically check the consistency of output for a collection of
.vpyfiles. New test case can now be added simply by adding new.vpyfiles.Fixes #501
Checklist
blackandisort)mypyreturns no errorpytestsucceedshelpreference.rstmake clean && make htmlindocs/)