[ENH] Add filename template legend #1219

Remi-Gau · 2022-08-17T08:35:15Z

closes #1198
fixes #1220

unrelated

update install of bidsschema tool via requirements
add black, import sort and flake8 to pre-commit config
run black and import sort on non schema code python
add setup.cfg with flake8 config in root folder to tell pre-commit what config to use

sappelhoff

could you please also remove:

bids-specification/.circleci/config.yml

Line 16 in d849c40

pip install ~/project/tools/schemacode/[render]

and re:

[ENH] Validator inheritance support #1120

maybe there is a way to make the JSON pre-commit hook ignore certain files?

Remi-Gau · 2022-08-17T09:05:36Z

and re:
* #1120
maybe there is a way to make the JSON pre-commit hook ignore certain files?

not for this specific hook, but will exclude the file globally in the pre-commit config
https://github.com/pre-commit/pre-commit-hooks#check-json

codecov · 2022-08-17T09:23:03Z

Codecov Report

Merging #1219 (86a7c30) into master (db4b0f5) will decrease coverage by 0.06%.
The diff coverage is 81.81%.

❗ Current head 86a7c30 differs from pull request most recent head 25574bd. Consider uploading reports for the commit 25574bd to get more accurate results

@@            Coverage Diff             @@
##           master    #1219      +/-   ##
==========================================
- Coverage   88.23%   88.16%   -0.07%     
==========================================
  Files           6        6              
  Lines        1020     1031      +11     
==========================================
+ Hits          900      909       +9     
- Misses        120      122       +2

Impacted Files	Coverage Δ
tools/schemacode/bidsschematools/render.py	`92.87% <81.81%> (-0.31%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Remi-Gau · 2022-08-17T09:33:21Z

weird note: running black . (blacK 22.6) to lint all files reformat some files and this will be undone by the black pre-commit hook.

Most likely not ideal but probably best to investigate somewhere else than in this PR

Example

``` datalad run -m 'run black on all files' 'black .' [INFO ] == Command start (output follows) ===== Skipping .ipynb files as Jupyter dependencies are not installed. You can fix this by running ``pip install black[jupyter]`` reformatted tools/schemacode/bidsschematools/tests/test_rules.py reformatted tools/schemacode/bidsschematools/tests/test_render.py reformatted tools/schemacode/bidsschematools/utils.py reformatted tools/schemacode/bidsschematools/schema.py reformatted tools/schemacode/bidsschematools/tests/test_validator.py reformatted tools/schemacode/bidsschematools/render.py reformatted tools/schemacode/bidsschematools/validator.py

All done! ✨ 🍰 ✨
7 files reformatted, 18 files left unchanged.
[INFO ] == Command exit (modification check follows) =====
add(ok): tools/schemacode/bidsschematools/render.py (file)
add(ok): tools/schemacode/bidsschematools/schema.py (file)
add(ok): tools/schemacode/bidsschematools/tests/test_render.py (file)
add(ok): tools/schemacode/bidsschematools/tests/test_rules.py (file)
add(ok): tools/schemacode/bidsschematools/tests/test_validator.py (file)
add(ok): tools/schemacode/bidsschematools/utils.py (file)
add(ok): tools/schemacode/bidsschematools/validator.py (file)
Total: 0%| | 0.00/1.00 [00:00<?, ? datasets/s]CommandError: 'git -c diff.ignoreSubmodules=none commit -m '[DATALAD RUNCMD] run black on all files

=== Do not change lines below ===
{
"chain": [],
"cmd": "black .",
"exit": 0,
"extra_inputs": [],
"inputs": [],
"outputs": [],
"pwd": "."
}
^^^ Do not change lines above ^^^
' --' failed with exitcode 1 under /home/remi/github/BIDS-specification
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...........................................(no files to check)Skipped
check json...........................................(no files to check)Skipped
check python ast.........................................................Passed
check for added large files..............................................Passed
check for case conflicts.................................................Passed
black....................................................................Failed

hook id: black
files were modified by this hook

reformatted tools/schemacode/bidsschematools/tests/test_rules.py
reformatted tools/schemacode/bidsschematools/tests/test_render.py
reformatted tools/schemacode/bidsschematools/utils.py
reformatted tools/schemacode/bidsschematools/schema.py
reformatted tools/schemacode/bidsschematools/tests/test_validator.py
reformatted tools/schemacode/bidsschematools/validator.py
reformatted tools/schemacode/bidsschematools/render.py

All done! ✨ 🍰 ✨
7 files reformatted.

flake8...................................................................Passed

</details>

Remi-Gau · 2022-08-17T09:36:41Z

@tsalo
when you have time could you check this weird horror that this PR is doing?

https://bids-specification--1219.org.readthedocs.build/en/1219/04-modality-specific-files/03-electroencephalography.html#eeg-recording-data

Now the filename templates include ALL datatypes everytime and the legend is rendered as codeblock. I must be missing something obvious but I can't see what.

Remi-Gau · 2022-08-17T13:21:58Z

Now the filename templates include ALL datatypes every time

hum this seems to actually already be the case in the live latest version:

https://bids-specification.readthedocs.io/en/latest/04-modality-specific-files/01-magnetic-resonance-imaging-data.html#anatomy-imaging-data

tsalo · 2022-08-17T13:25:45Z

~~Latest looks fine to me.~~ Never mind, I was looking at stable! 🤦 I'll try to figure out what's going on.

EDIT: My first guess is that the filter_schema function doesn't work with the new Namespace objects (see #1113).

tsalo · 2022-08-17T13:42:23Z

I think I have the problem pinned down, but I don't have a solution yet.

The problem is that filter_schema checks for dict objects, instead of Namespace objects (which the schema now is). Changing dict below to Mapping addresses that problem.

bids-specification/tools/schemacode/bidsschematools/schema.py

Lines 260 to 279 in d849c40

    
           new_schema = deepcopy(schema) 
        
           if isinstance(new_schema, dict): 
        
               # Reduce values in dict to only requested 
        
               for k, v in kwargs.items(): 
        
                   if k in new_schema.keys(): 
        
                       filtered_item = deepcopy(new_schema[k]) 
        
                       if isinstance(filtered_item, dict): 
        
                           filtered_item = {k1: v1 for k1, v1 in filtered_item.items() if k1 in v} 
        
                       else: 
        
                           filtered_item = [i for i in filtered_item if i in v] 
        
                       new_schema[k] = filtered_item 
        
                   for k2, v2 in new_schema.items(): 
        
                       new_schema[k2] = filter_schema(new_schema[k2], **kwargs) 
        
           elif isinstance(new_schema, list): 
        
               for i, item in enumerate(new_schema): 
        
                   if isinstance(item, dict): 
        
                       new_schema[i] = filter_schema(item, **kwargs) 
        
           return new_schema

The next problem is that Namespace objects do not support item assignment, and filter_schema relies heavily on item assignment (e.g., new_schema[k] = filtered_item) to build the filtered version of the schema. I haven't figured out how to work around this. @effigies do you have any ideas for how to address the item assignment problem?

Remi-Gau · 2022-08-17T13:54:45Z

Thanks for looking into this.

Should we add a test to catch this in ci rather than having to manually check the rendering?

Also because running mkdocs serves takes "ages" to run locally which does not incentive to check.

tsalo · 2022-08-17T14:10:00Z

Chris had some ideas for fixing the filter that we talked about briefly on Zoom. I'll be trying them out once I finish up with a call I'm on.

Should we add a test to catch this in ci rather than having to manually check the rendering?

We do have #788 that seems similar, but I never figured out how to actually implement it. Otherwise, I think we'll need to write unit tests for the rendering functions. I think Horea added some, but they must not be strict enough to catch this new problem.

effigies · 2022-08-17T14:10:41Z

I'll try to make a quick fix.

effigies · 2022-08-17T14:27:12Z

See #1223

tools/schemacode/bidsschematools/render.py

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Remi-Gau · 2022-08-17T20:38:27Z

Thanks @tsalo @effigies for looking into this and fixing all of that.

@tsalo did I miss them or are the label and index definition not in the glossary?

Remi-Gau · 2022-08-17T20:40:58Z

We do have #788 that seems similar, but I never figured out how to actually implement it. Otherwise, I think we'll need to write unit tests for the rendering functions. I think Horea added some, but they must not be strict enough to catch this new problem.

I may have a look into this see if I can think of something. Don't hold your breath.

tools/schemacode/bidsschematools/render.py

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Remi-Gau · 2022-08-24T13:04:57Z

PDF and HTML now look good to me.

tools/schemacode/bidsschematools/render.py

sappelhoff · 2022-08-25T09:19:08Z

.pre-commit-config.yaml

+-   repo: https://github.com/asottile/reorder_python_imports
+    rev: "v3.8.2"
+    hooks:
+    -   id: reorder-python-imports


why do you prefer this over the more standard isort?

.pre-commit-config.yaml

sappelhoff · 2022-08-25T09:20:56Z

setup.cfg

+# this config is mostly there to help with the use of pre-commit
+max-line-length = 110
+ignore = E203,E402,E722,W503,F841
+docstring-convention = numpy


I think for this to work we would need https://pypi.org/project/flake8-docstrings/ ... correct?

We could add it to requirements.txt and then also add it to the pre-commit hook like here: https://github.com/sappelhoff/pyprep/blob/25b91e098d0055f14b5cbcae54b5ee9e0739aa62/.pre-commit-config.yaml#L27

Hum...

I have been "wasting" some time because the linting rules of precommit and those of the ci the bidsschematools clashed, so more than happy to try to make them agree.

However I know hate this PR because it does too. Many. Things.

So I will remove all changes that are not related to the title of this PR and put them in a different one.

It's also fine with me to merge this as is and then iterate, no worries.

setup.cfg is really meant for Python packages, which this isn't. If we're going to have it at the root, I would rather call it .flake8. See https://flake8.pycqa.org/en/latest/user/configuration.html

Remi-Gau · 2022-08-25T10:23:06Z

Yeah but it bothers me.

So expect a force push.

Remi-Gau · 2022-08-25T10:31:50Z

Yeah but it bothers me.

So expect a force push.

OK I changed my mind: no force push. With several merge of master into this, reset and sorting what belongs to this PR or to another is going to be a pain.

So I would prefer merging and fix the pre-commit config in another PR. (pragmatism beats clean commit history on this one)

sappelhoff · 2022-08-25T10:35:01Z

So I would prefer merging and fix the pre-commit config in another PR. (pragmatism beats clean commit history on this one)

yes 👍 especially because we can squash and merge it

sappelhoff

Just to clarify - are you ready to have this merged @Remi-Gau ?

Remi-Gau · 2022-08-25T11:14:18Z

Just to clarify - are you ready to have this merged @Remi-Gau ?

yup if @tsalo and @effigies are OK with it

tsalo

Sorry for the delay. I'm happy with this as-is.

effigies

TBH I'm finding this hard to review because of bundling process changes with semantic changes. I made a couple small comments regarding the render, which seems fine, and the contributing guide. If everything else is just process, that's fine.

tools/schemacode/bidsschematools/render.py

effigies · 2022-08-29T16:08:15Z

requirements.txt

@@ -4,4 +4,4 @@ pymdown-extensions>=7.0.0
 mkdocs-branchcustomization-plugin~=0.1.3
 mkdocs-macros-plugin
 numpy
-tools/schemacode/[render]
+-e tools/schemacode/[render]


I think I suggested this, but I disagree now. This is part of the build process in RTD, and we shouldn't rely on -e.

Suggested change

-e tools/schemacode/[render]

tools/schemacode/[render]

effigies · 2022-08-29T16:10:46Z

CONTRIBUTING.md

@@ -302,7 +302,6 @@ is to use the `requirements.txt` file contained in this repository as follows:
 ```bash
 pip install -U pip
 pip install -r requirements.txt
-pip install -e tools/schemacode/


So this should be restored.

effigies · 2022-08-29T16:37:23Z

pdf_build_src/process_markdowns.py

@@ -24,11 +23,12 @@

 def run_shell_cmd(command):
    """Run shell/bash commands passed as a string using subprocess module."""
-    process = subprocess.Popen(command.split(), stdout=subprocess.PIPE,
-                               stderr=subprocess.PIPE)
+    process = subprocess.Popen(


Are there changes in this file? Style rerender?

effigies · 2022-08-29T16:46:31Z

setup.cfg

+# this config is mostly there to help with the use of pre-commit
+max-line-length = 110
+ignore = E203,E402,E722,W503,F841
+docstring-convention = numpy


setup.cfg is really meant for Python packages, which this isn't. If we're going to have it at the root, I would rather call it .flake8. See https://flake8.pycqa.org/en/latest/user/configuration.html

Remi-Gau · 2022-08-29T17:01:33Z

@effigies
I can close this and start a cleaner PR, if you prefer.
This thing has become a headache to review and to work with TBF. Sorry. :-(

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

effigies · 2022-08-29T17:11:38Z

Sorry, I hate to be annoying, but I really would prefer to keep these things separate. If it would save you trouble, I'd be happy to unsentimentally squash what I think are the relevant changes into a single commit with you as author, and you can just review that.

Remi-Gau · 2022-08-29T17:14:29Z

Sorry, I hate to be annoying, but I really would prefer to keep these things separate. If it would save you trouble, I'd be happy to unsentimentally squash what I think are the relevant changes into a single commit with you as author, and you can just review that.

No worries. I was getting annoyed at how this PR was started to mix things that were unrelated but I think I gave up too quickly to sort it.

Will close and open separate PRs.

Remi-Gau added 2 commits August 17, 2022 10:31

install tools as editable via requirements

f49ca5e

update doc

d57614e

Remi-Gau requested review from tsalo and erdalkaraca as code owners August 17, 2022 08:35

add legend

453fef6

Remi-Gau force-pushed the add_legend branch from 0678ed1 to 453fef6 Compare August 17, 2022 09:00

sappelhoff reviewed Aug 17, 2022

View reviewed changes

update CI

c283e2c

Remi-Gau added 3 commits August 17, 2022 11:11

add black to pre-commit and apply

b76547b

add flake8 to precommit

a33f3da

lint

866e248

Merge branch 'master' into add_legend

b7adfcb

tsalo reviewed Aug 17, 2022

View reviewed changes

tools/schemacode/bidsschematools/render.py Outdated Show resolved Hide resolved

Remi-Gau and others added 2 commits August 17, 2022 22:17

Update tools/schemacode/bidsschematools/render.py

6e28924

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Merge branch 'master' into add_legend

718e2b3

effigies reviewed Aug 17, 2022

View reviewed changes

tools/schemacode/bidsschematools/render.py Outdated Show resolved Hide resolved

Update tools/schemacode/bidsschematools/render.py

bf6ad1f

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Remi-Gau requested a review from effigies August 24, 2022 13:04

effigies reviewed Aug 24, 2022

View reviewed changes

tools/schemacode/bidsschematools/render.py Outdated Show resolved Hide resolved

use markdown render

8f086b8

Remi-Gau force-pushed the add_legend branch 2 times, most recently from 1e34204 to ec9e7d8 Compare August 24, 2022 15:59

add python import sort to pre commit and apply to non-schema code

7169abf

Remi-Gau force-pushed the add_legend branch from ec9e7d8 to 7169abf Compare August 24, 2022 16:02

Remi-Gau added 2 commits August 24, 2022 18:05

lint

f7625cd

Merge branch 'master' into add_legend

86a7c30

sappelhoff reviewed Aug 25, 2022

View reviewed changes

sappelhoff approved these changes Aug 25, 2022

View reviewed changes

sappelhoff requested a review from effigies August 25, 2022 11:18

tsalo approved these changes Aug 29, 2022

View reviewed changes

effigies reviewed Aug 29, 2022

View reviewed changes

Remi-Gau and others added 3 commits August 29, 2022 19:10

Update tools/schemacode/bidsschematools/render.py

06ff974

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Update .pre-commit-config.yaml

a70c060

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>

Update tools/schemacode/bidsschematools/render.py

25574bd

Co-authored-by: Chris Markiewicz <effigies@gmail.com>

Remi-Gau closed this Aug 29, 2022

This was referenced Aug 29, 2022

[ENH] Add filename template legend #1259

Merged

[DOC] minor change on install documentation and for circle CI #1266

Merged

[IGNORE] setup precommit to play nice with bidsschema CI #1267

Closed

sappelhoff mentioned this pull request Oct 14, 2022

Fix pre-commit hooks for broken JSON #1320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Add filename template legend #1219

[ENH] Add filename template legend #1219

Remi-Gau commented Aug 17, 2022 •

edited

Loading

sappelhoff left a comment

Remi-Gau commented Aug 17, 2022

codecov bot commented Aug 17, 2022 •

edited

Loading

Remi-Gau commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

tsalo commented Aug 17, 2022 •

edited

Loading

tsalo commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

tsalo commented Aug 17, 2022

effigies commented Aug 17, 2022

effigies commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

Remi-Gau commented Aug 24, 2022

sappelhoff Aug 25, 2022

sappelhoff Aug 25, 2022

Remi-Gau Aug 25, 2022

sappelhoff Aug 25, 2022

effigies Aug 29, 2022

Remi-Gau commented Aug 25, 2022

Remi-Gau commented Aug 25, 2022

sappelhoff commented Aug 25, 2022

sappelhoff left a comment

Remi-Gau commented Aug 25, 2022

tsalo left a comment

effigies left a comment

effigies Aug 29, 2022

effigies Aug 29, 2022

effigies Aug 29, 2022

effigies Aug 29, 2022

Remi-Gau commented Aug 29, 2022

effigies commented Aug 29, 2022

Remi-Gau commented Aug 29, 2022

[ENH] Add filename template legend #1219

[ENH] Add filename template legend #1219

Conversation

Remi-Gau commented Aug 17, 2022 • edited Loading

unrelated

sappelhoff left a comment

Choose a reason for hiding this comment

Remi-Gau commented Aug 17, 2022

codecov bot commented Aug 17, 2022 • edited Loading

Codecov Report

Remi-Gau commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

tsalo commented Aug 17, 2022 • edited Loading

tsalo commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

tsalo commented Aug 17, 2022

effigies commented Aug 17, 2022

effigies commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

Remi-Gau commented Aug 17, 2022

Remi-Gau commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Aug 25, 2022

Remi-Gau commented Aug 25, 2022

sappelhoff commented Aug 25, 2022

sappelhoff left a comment

Choose a reason for hiding this comment

Remi-Gau commented Aug 25, 2022

tsalo left a comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Aug 29, 2022

effigies commented Aug 29, 2022

Remi-Gau commented Aug 29, 2022

Remi-Gau commented Aug 17, 2022 •

edited

Loading

codecov bot commented Aug 17, 2022 •

edited

Loading

tsalo commented Aug 17, 2022 •

edited

Loading