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.

Sign up for GitHub

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

Jump to bottom

Separate Parsec ItemNotFoundError into two #4471

Merged
merged 18 commits into from
Nov 16, 2021
Merged

Separate Parsec ItemNotFoundError into two #4471

merged 18 commits into from
Nov 16, 2021

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Oct 18, 2021 •
edited by oliver-sanders
Loading

Before
ItemNotFoundError was returned if

  • The item wasn't defined in the sparse config.
  • The item wasn't available to be defined, because the item was mis-spelled or totally non-existant.

After
ItemNotFoundError - Where an item could be defined, but isn't (in sparse mode).
NotAConfigItemError - Where an item is not part of a config to be defined.

These changes close #4187

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.py and
    conda-environment.yml.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@wxtim wxtim marked this pull request as draft October 18, 2021 15:03
@wxtim wxtim self-assigned this Oct 19, 2021
@wxtim wxtim marked this pull request as ready for review October 19, 2021 08:06
@wxtim
Separate Parsec ItemNotFoundError into two
08155bf 
ItemNotFoundError - Where an item could be defined, but isn't (in sparse mode).
NotAConfigItemError - Where an item is not part of a config to be defined.

move get_manyparents to init

create unit test for get_manyparents

fix mypy issue
@wxtim wxtim added could be better Not exactly a bug, but not ideal. small labels Oct 19, 2021
@wxtim wxtim added this to the cylc-8.0rc1 milestone Oct 19, 2021
oliver-sanders
oliver-sanders approved these changes Oct 19, 2021
View reviewed changes
cylc/flow/parsec/exceptions.py Outdated Show resolved Hide resolved
cylc/flow/parsec/exceptions.py Outdated Show resolved Hide resolved
@wxtim @oliver-sanders
Update cylc/flow/parsec/exceptions.py
cf33544 
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
wxtim
wxtim commented Oct 19, 2021
View reviewed changes
cylc/flow/parsec/config.py Outdated Show resolved Hide resolved
wxtim
wxtim commented Oct 19, 2021
View reviewed changes
cylc/flow/parsec/config.py Outdated Show resolved Hide resolved
wxtim and others added 2 commits October 19, 2021 11:16
@wxtim @oliver-sanders
Apply suggestions from code review
73c1568 
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
@wxtim
Update cylc/flow/parsec/config.py
3ec13e1
MetRonnie
MetRonnie requested changes Oct 22, 2021
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
cylc/flow/parsec/config.py Outdated Show resolved Hide resolved
cylc/flow/parsec/config.py Outdated Show resolved Hide resolved
cylc/flow/parsec/config.py Outdated Show resolved Hide resolved
cylc/flow/parsec/exceptions.py Outdated Show resolved Hide resolved
wxtim and others added 3 commits October 25, 2021 10:01
@wxtim @MetRonnie
Update CHANGES.md
cd27c3e 
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
@wxtim @MetRonnie
Update cylc/flow/parsec/config.py
5bb4250 
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
@wxtim
fixed logic
2596840
@wxtim wxtim requested a review from MetRonnie October 25, 2021 10:06
MetRonnie
MetRonnie reviewed Oct 29, 2021
View reviewed changes
cylc/flow/parsec/config.py Outdated Show resolved Hide resolved
tests/unit/parsec/test_config.py Outdated Show resolved Hide resolved
tests/unit/parsec/test_config.py Show resolved Hide resolved
@MetRonnie
Copy link
Member

Something very odd has happened to this branch (discontinuity between the commit named fixed the logic further and pr/wxtim/4471-1):

image

I'm going to try re-merging master into it and force-pushing

@MetRonnie MetRonnie mentioned this pull request Nov 1, 2021
Merged
@MetRonnie
Copy link
Member

I've also thought of a way to simplify the logic - opened a PR against this one at wxtim#32

wxtim and others added 3 commits November 2, 2021 09:03
@wxtim @MetRonnie
Apply suggestions from code review
0524869 
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
@wxtim
review response
0a47fdd
@wxtim
Merge branch 'config.fix' of github.com:wxtim/cylc into config.fix
de0da57 
* 'config.fix' of github.com:wxtim/cylc: (71 commits)
  Apply suggestions from code review
  Tweak TUI job prep state icon.
  Reduce test flakiness on MacOS
  Fix logging of task state transistion to "preparing".
  Fix datastore job "preparing" state.
  Fix TUI job "preparing" state.
  Fix spelling error.
  Func test fix.
  Style fix.
  Workflow config doc refinements (cylc#4475)
  Update cylc/flow/cfgspec/workflow.py
  Fix two func tests, and a typo.
  Remove CYLC_WORKFLOW_DEF_PATH
  Address review feedback (tweak some docstrings).
  Update cylc/flow/task_pool.py
  Post rebase fix.
  Set flow counter from DB.
  Fix a func test post rebase.
  Fix cylc poll CLI doc.
  Add nosec comment.
  ...
@wxtim wxtim requested a review from MetRonnie November 2, 2021 09:17
@MetRonnie
Copy link
Member

Rebased wxtim#32 on latest changes

@MetRonnie
Copy link
Member

(Still waiting for outcome of wxtim#32)

@wxtim
Merge pull request #32 from MetRonnie/patch-pr-4471
4a122b9 
Simplify `ParsecConfig.get()`
@wxtim
Copy link
Member Author

wxtim commented Nov 15, 2021

(Still waiting for outcome of wxtim#32)

Sorry, missed that you'd not made the changes directly to the branch but had created a PR to PR.

MetRonnie
MetRonnie reviewed Nov 15, 2021
View reviewed changes
CHANGES.md Outdated Show resolved Hide resolved
MetRonnie
MetRonnie approved these changes Nov 15, 2021
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Happy assuming tests pass

@wxtim wxtim merged commit fd37bb1 into cylc:master Nov 16, 2021
@wxtim wxtim deleted the config.fix branch November 16, 2021 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MetRonnie MetRonnie MetRonnie approved these changes

@oliver-sanders oliver-sanders Awaiting requested review from oliver-sanders

Assignees

@wxtim wxtim

Labels
could be better Not exactly a bug, but not ideal. small
Projects
None yet
Milestone
cylc-8.0rc1
Development

Successfully merging this pull request may close these issues.

cylc config --sparse: item not found
3 participants
@wxtim @MetRonnie @oliver-sanders