-
Notifications
You must be signed in to change notification settings - Fork 169
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
[SCHEMA] Reorganize schema code into a package #892
Conversation
I don't like this. We need something less dependent on folder locations.
It works locally...
I'm not sure why mkdocs has such a hard time tracking down the schema files in CircleCI. It works fine locally... EDIT: Of course, it's because, now that |
My main blocker at the moment is that, as a package, |
ce389b1
to
fadc12c
Compare
@tsalo Made several updates. RTD is fixed. The PDF build looks like it needs updating to new modules. I assume you'll be able to figure that out faster than me, but let me know if you want me to tackle it. |
Thanks @effigies! How does it work? It looks like the schema files are duplicated in |
It's just a symlink, so they will be copied into the package. If you're going to develop, it probably makes sense to |
Awesome! I fixed the module paths, so I think everything is good to go. |
@bendhouseart sorry for not requesting your review earlier. I know you wanted to take a look, though we didn't end up trying out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the messed up links only appear in HTML comments in the built site.
I see :| but it's still annoying to have linkchecker now fail until we release ...
But that's not a blocker for this PR, which I approve of!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some minor comments/suggestions - ok to ignore
Co-authored-by: Yaroslav Halchenko <[email protected]>
We could add a small Python script that checks whether the current commit is tagged (=stable), or not (=dev), and then in CI before building with mkdocs, that Python scripts changes Line 2 in 98ca3fa
🤔 or would we screw up search engine results with that? I am not very familiar with "canonical URL" metadata in HTML headers. |
I have no clue if that's possible, but having the ability to update content files with Python scripts within CI would be really useful for adding tooltips (#954). The problem I have been bumping into for that is that the macros-generated content is not available for the auto-linked tooltips to grab onto. Generating the glossary within CI would circumvent that issue, I believe. That said, I don't even know where to start to do it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked over the changed files and managed to build and install locally w/ setup.py 👍. I'd call it good to go.
Python 3.9.9 (main, Nov 21 2021, 03:22:47)
[Clang 12.0.0 (clang-1200.0.32.29)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import schemacode
>>>
CircleCI disagrees, but eh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A couple bits of code caught my eye as likely able to be simplified. I haven't tested though...
Thanks for the suggestions! Co-authored-by: Chris Markiewicz <[email protected]>
The built site looks good to me, as does the PDF. Now that we have two approvals, is this alright to merge or do we want to wait 5 days? Also, thank you to @effigies and @bendhouseart for your reviews! |
The 5 days is more for semantic changes than infrastructure. |
This stems from #885 (comment). The idea is to make the schema code operate as a proper Python package, so we can also start writing tests for it.
We still may want to move schema-related code into a different repository at some point (e.g.,
bids-schema
,pybids
), but for now I'd just like to have the code organized nicely within this repo.Changes proposed:
schemacode
into a Python package, so that it can be installed and tests can be written for it.src/schema
totools/schemacode/schemacode/data/schema
. Thanks to @effigies for that.schemacode
. These can be improved in future PRs.