juaml/junifer
Watch
3
0
Fork
You've already forked junifer
 0
Code Issues 14 Pull requests 2 Packages Activity Actions

Use ruamel.yaml in place of pyyaml #223

Merged
synchon merged 17 commits from update/ruamel.yaml into main 2023-05-09 11:07:29 +00:00
Conversation 15 Commits 17 Files changed 17 +73 -50
synchon commented 2023-04-24 12:00:30 +00:00 (Migrated from github.com)
Copy link

pyyaml is lacking two features which we are interested in:

  • Support for YAML 1.2
  • Multiline string

ruaml.yaml provides support for them, so will be worth using it over pyyaml.

`pyyaml` is lacking two features which we are interested in: - Support for YAML 1.2 - Multiline string `ruaml.yaml` provides support for them, so will be worth using it over `pyyaml`.
fraimondo (Migrated from github.com) reviewed 2023-04-24 12:00:30 +00:00
codecov[bot] commented 2023-04-24 12:03:11 +00:00 (Migrated from github.com)
Copy link

Codecov Report

Merging #223 (6d5eca1) into main (34035fa) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #223   +/-   ##
=======================================
  Coverage   93.60%   93.60%           
=======================================
  Files          80       80           
  Lines        3458     3458           
  Branches      653      650    -3     
=======================================
  Hits         3237     3237           
  Misses        144      144           
  Partials       77       77
Flag Coverage Δ
docs 100.00% <ø> (ø)
junifer 93.60% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
junifer/api/decorators.py 100.00% <ø> (ø)
junifer/api/cli.py 69.14% <100.00%> (-0.33%) ⬇️
junifer/api/functions.py 96.23% <100.00%> (-0.03%) ⬇️
junifer/api/parser.py 94.87% <100.00%> (-0.13%) ⬇️
junifer/api/utils.py 97.67% <100.00%> (+0.17%) ⬆️
junifer/utils/logging.py 71.42% <100.00%> (ø)
## [Codecov](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#223](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (6d5eca1) into [main](https://codecov.io/gh/juaml/junifer/commit/34035faf072e51967ae498219682cca7ad36839c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (34035fa) will **not change** coverage. > The diff coverage is `100.00%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/223/graphs/tree.svg?width=650&height=150&src=pr&token=5H21JuZXMw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #223 +/- ## ======================================= Coverage 93.60% 93.60% ======================================= Files 80 80 Lines 3458 3458 Branches 653 650 -3 ======================================= Hits 3237 3237 Misses 144 144 Partials 77 77 ``` | Flag | Coverage Δ | | |---|---|---| | docs | `100.00% <ø> (ø)` | | | junifer | `93.60% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/api/decorators.py](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvZGVjb3JhdG9ycy5weQ==) | `100.00% <ø> (ø)` | | | [junifer/api/cli.py](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvY2xpLnB5) | `69.14% <100.00%> (-0.33%)` | :arrow_down: | | [junifer/api/functions.py](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvZnVuY3Rpb25zLnB5) | `96.23% <100.00%> (-0.03%)` | :arrow_down: | | [junifer/api/parser.py](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvcGFyc2VyLnB5) | `94.87% <100.00%> (-0.13%)` | :arrow_down: | | [junifer/api/utils.py](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvdXRpbHMucHk=) | `97.67% <100.00%> (+0.17%)` | :arrow_up: | | [junifer/utils/logging.py](https://codecov.io/gh/juaml/junifer/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci91dGlscy9sb2dnaW5nLnB5) | `71.42% <100.00%> (ø)` | |
github-actions[bot] commented 2023-04-24 12:05:48 +00:00 (Migrated from github.com)
Copy link
PR Preview Action v0.0.2-71-g84ed73b3
Preview removed because the pull request was closed.
2023-05-09 11:12 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v0.0.2-71-g84ed73b3 :---: Preview removed because the pull request was closed. 2023-05-09 11:12 UTC <!-- Sticky Pull Request Commentpr-preview -->
LeSasse (Migrated from github.com) reviewed 2023-05-08 10:48:27 +00:00
LeSasse (Migrated from github.com) left a comment
Copy link

I just have a few clarification questions but that is it.

I just have a few clarification questions but that is it.
docs/faq.rst
@ -24,3 +24,3 @@
.. code-block:: bash
conda env create -n <your-environment-name> -f conda-env.yml python=3.10
conda env create -n <your-environment-name> -f conda-env.yml
LeSasse (Migrated from github.com) commented 2023-05-08 10:40:31 +00:00
Copy link

What is the reason for not having to specify the python version in the command anymore?

What is the reason for not having to specify the python version in the command anymore?
junifer/api/functions.py
LeSasse (Migrated from github.com) commented 2023-05-08 10:43:41 +00:00
Copy link

Why does this not need a context manager anymore?

Why does this not need a context manager anymore?
junifer/api/parser.py
LeSasse (Migrated from github.com) commented 2023-05-08 10:44:19 +00:00
Copy link

Same Q as above

Same Q as above
junifer/api/tests/test_functions.py
@ -263,4 +276,4 @@
generated_config_yaml_path = Path(
tmp_path / "junifer_jobs" / "yaml_config_gen_check" / "config.yaml"
)
LeSasse (Migrated from github.com) commented 2023-05-08 10:45:47 +00:00
Copy link

Should you use CamelCase in the docstring here, similar to the DataGrabber issue?

Should you use CamelCase in the docstring here, similar to the DataGrabber issue?
junifer/api/tests/test_functions.py
@ -267,3 +280,2 @@
with open(generated_config_yaml_path, "r") as f:
yaml_config = yaml.unsafe_load(f)
yaml_config = yaml.load(generated_config_yaml_path)
# Check for correct YAML config generation
LeSasse (Migrated from github.com) commented 2023-05-08 10:45:36 +00:00
Copy link

Should you use CamelCase in the docstring here, similar to the DataGrabber issue?

Should you use CamelCase in the docstring here, similar to the DataGrabber issue?
junifer/api/utils.py
@ -16,0 +19,4 @@
yaml = YAML()
yaml.default_flow_style = False
yaml.allow_unicode = True
yaml.indent(mapping=2, sequence=4, offset=2)
LeSasse (Migrated from github.com) commented 2023-05-08 10:47:42 +00:00
Copy link

So this is a global object that you import elsewhere to load files?

So this is a global object that you import elsewhere to load files?
synchon (Migrated from github.com) reviewed 2023-05-08 11:42:26 +00:00
junifer/api/functions.py
synchon (Migrated from github.com) commented 2023-05-08 11:42:26 +00:00
Copy link

That's cause ruamel.yaml allows you to write to files via stream argument in the dump function.

That's cause `ruamel.yaml` allows you to write to files via `stream` argument in the `dump` function.
synchon (Migrated from github.com) reviewed 2023-05-08 11:43:55 +00:00
junifer/api/tests/test_functions.py
@ -267,3 +280,2 @@
with open(generated_config_yaml_path, "r") as f:
yaml_config = yaml.unsafe_load(f)
yaml_config = yaml.load(generated_config_yaml_path)
# Check for correct YAML config generation
synchon (Migrated from github.com) commented 2023-05-08 11:43:55 +00:00
Copy link

Yeah I was debating that and I followed what was there but fair point. I'll take care of that in a separate PR for all test functions.

Yeah I was debating that and I followed what was there but fair point. I'll take care of that in a separate PR for all test functions.
synchon (Migrated from github.com) reviewed 2023-05-08 11:44:59 +00:00
junifer/api/utils.py
@ -16,0 +19,4 @@
yaml = YAML()
yaml.default_flow_style = False
yaml.allow_unicode = True
yaml.indent(mapping=2, sequence=4, offset=2)
synchon (Migrated from github.com) commented 2023-05-08 11:44:59 +00:00
Copy link

Yeah cause now you gotta have an instance of YAML class so doesn't make sense to create multiple objects if you have the same thing everywhere.

Yeah cause now you gotta have an instance of `YAML` class so doesn't make sense to create multiple objects if you have the same thing everywhere.
LeSasse (Migrated from github.com) reviewed 2023-05-08 12:56:18 +00:00
junifer/api/tests/test_functions.py
@ -267,3 +280,2 @@
with open(generated_config_yaml_path, "r") as f:
yaml_config = yaml.unsafe_load(f)
yaml_config = yaml.load(generated_config_yaml_path)
# Check for correct YAML config generation
LeSasse (Migrated from github.com) commented 2023-05-08 12:56:17 +00:00
Copy link

ok makes sense, fine by me!

ok makes sense, fine by me!
LeSasse (Migrated from github.com) reviewed 2023-05-08 12:56:35 +00:00
junifer/api/functions.py
LeSasse (Migrated from github.com) commented 2023-05-08 12:56:35 +00:00
Copy link

and this takes care of closing the ressource?

and this takes care of closing the ressource?
synchon (Migrated from github.com) reviewed 2023-05-08 13:01:07 +00:00
junifer/api/functions.py
synchon (Migrated from github.com) commented 2023-05-08 13:01:07 +00:00
Copy link

Yes exactly.

Yes exactly.
synchon (Migrated from github.com) reviewed 2023-05-08 13:25:45 +00:00
docs/faq.rst
@ -24,3 +24,3 @@
.. code-block:: bash
conda env create -n <your-environment-name> -f conda-env.yml python=3.10
conda env create -n <your-environment-name> -f conda-env.yml
synchon (Migrated from github.com) commented 2023-05-08 13:25:45 +00:00
Copy link

As we have that constraint (python=3.10) in the conda-env.yml and also this syntax is incorrect from what I remember.

As we have that constraint (`python=3.10`) in the `conda-env.yml` and also this syntax is incorrect from what I remember.
synchon (Migrated from github.com) reviewed 2023-05-08 13:27:40 +00:00
junifer/api/parser.py
synchon (Migrated from github.com) commented 2023-05-08 13:27:40 +00:00
Copy link

Same argument as above with the only change that load can take care of opening the file.

Same argument as above with the only change that `load` can take care of opening the file.
synchon (Migrated from github.com) reviewed 2023-05-08 13:30:44 +00:00
junifer/api/tests/test_functions.py
@ -263,4 +276,4 @@
generated_config_yaml_path = Path(
tmp_path / "junifer_jobs" / "yaml_config_gen_check" / "config.yaml"
)
synchon (Migrated from github.com) commented 2023-05-08 13:30:43 +00:00
Copy link

Resolving this as I'll take care of this in a separate PR.

Resolving this as I'll take care of this in a separate PR.
LeSasse (Migrated from github.com) reviewed 2023-05-08 13:51:20 +00:00
LeSasse (Migrated from github.com) left a comment
Copy link

Ship it!

Ship it!
LeSasse (Migrated from github.com) approved these changes 2023-05-09 10:35:54 +00:00
LeSasse (Migrated from github.com) left a comment
Copy link

Approved, thanks for your hard work on this!

Approved, thanks for your hard work on this!
Sign in to join this conversation.
Reviewers
No reviewers
fraimondo
LeSasse
Labels
Clear labels   
CRITICAL
High Priority Issue

  
Stale

   
WIP
Work in progress

  
bug
Something isn't working

  
concept
Design/Implementation concepts

  
coordinate
Issues or pull requests related to coordinates (ROIs)

  
dataset
Issues or pull requests related to datagrabbers or datasets

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
maintenance
Issues or pull requests related to maintenance

  
maps
Issues or pull requests related to maps (probabilistic atlases)

  
marker
Issues or pull requests related to markers

  
mask
Issues or pull requests related to masks

  
on hold
Issue waiting on other issue(s)

  
parcellation
Issues or pull requests related to parcellations (deterministic atlases)

  
preprocess
Issues or pull requests related to preprocessors

  
question
Further information is requested

  
ready
Pull request is ready for review and merge

  
storage
Issues or pull requests related to marker storage or retrieval

  
template-space
Issues or pull requests related to template spaces

  
triage
New issues waiting to be reviewed

  
wontfix
This will not be worked on

No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
Milestone
Clear milestone
No items
No milestone
0.0.3 (alpha 2)
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
juaml/junifer!223
No description provided.