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

Incrementally edit manifest on alr with #484

Merged
merged 5 commits into from
Aug 7, 2020
Merged

Conversation

mosteo
Copy link
Member

@mosteo mosteo commented Jul 28, 2020

This is the change described in #466 in which alr with doesn't regenerate the whole manifest but only appends new dependencies at the end.

Adding dependencies is trivial; removal is allowed for static dependencies (the kind that can be added via alr with anyway) and will fail for a dynamic dependency introduced manually or any other unforeseen circumstance. Still, dynamic and static dependencies can now coexist in a manifest and alr with will not complain, unlike before where a single dynamic dependency would preclude alr with from working anymore (the manifest could not be regenerated).

Along the way, this PR introduces a couple of supporting types to allow safe editing of the manifest, in the sense that any error while adding/removing dependencies to the manifest will preserve the original manifest, untouched, just in case.

Also, the changes surfaced the circumstance that solved dependencies are always shown in alphabetical order, whereas direct unsolved dependencies were shown in their loading order. The latter have been fixed for consistency.

@mosteo mosteo force-pushed the feat/append branch 3 times, most recently from 4bf24de to 0f4bac8 Compare July 31, 2020 06:20
@mosteo mosteo marked this pull request as ready for review July 31, 2020 06:57
@mosteo mosteo requested a review from Fabien-Chouteau July 31, 2020 07:04
@mosteo mosteo force-pushed the feat/separate-origins branch from 3cb3d3f to 5e92a6c Compare August 7, 2020 12:36
Base automatically changed from feat/separate-origins to merging/big-refactor August 7, 2020 12:44
mosteo added 5 commits August 7, 2020 14:46
This action will also not touch lines that aren't involved in the deletion.
Only a simple dependency, and trivially empty [[depends-on]] array entries will
be removed. As an improvement over the previous situation, dynamic expressions
will not preclude using `alr with` for static dependencies, leaving the dynamic
ones untouched.
This was happening before by chance in several tests. The `alr with` changes
have surfaced that the order on file is kept in tree; while this has no
secondary effects, for presentation to the user is better to be always
consistent. Solutions already use ordered sets, so for lists within conditional
trees we do the same (only for presentation).
@mosteo mosteo merged commit 7d684e8 into merging/big-refactor Aug 7, 2020
@mosteo mosteo deleted the feat/append branch August 7, 2020 12:56
mosteo added a commit that referenced this pull request Sep 1, 2020
* Append new dependencies to the end of the manifest

* Conservative removal of dependencies in `alr with`

This action will also not touch lines that aren't involved in the deletion.
Only a simple dependency, and trivially empty [[depends-on]] array entries will
be removed. As an improvement over the previous situation, dynamic expressions
will not preclude using `alr with` for static dependencies, leaving the dynamic
ones untouched.

* Use lexicographical order for printing dependencies

This was happening before by chance in several tests. The `alr with` changes
have surfaced that the order on file is kept in tree; while this has no
secondary effects, for presentation to the user is better to be always
consistent. Solutions already use ordered sets, so for lists within conditional
trees we do the same (only for presentation).

* Testsuite minor output fix

* Tests for new removal of dependencies
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.

2 participants