[INFRA] fix spec-internal links from schema entries #1056

sappelhoff · 2022-04-09T17:13:02Z

update the new way works, see comments further below.

fixed for:

env.macro(macros.make_glossary, "MACROS___make_glossary")
env.macro(macros.make_suffix_table, "MACROS___make_suffix_table")
env.macro(macros.make_metadata_table, "MACROS___make_metadata_table")
env.macro(macros.make_columns_table, "MACROS___make_columns_table")

attempt to implement solution for #974

This doesn't work as I expected, because (I think) two passes to render macros would be needed:

to paste a schema entry into a table
to change a link within that pasted table

Currently only step 1. happens, and step 2. remains unresolved, with the {{ xxx }} schema syntax in the table. I guess this was to be expected - do you have an idea how to circumvent this with the existing infrastructure @tsalo?

Another way I can think of is to make this "link fixing" a special citizen in the form of a python script that we run on the built files (after mkdocs build).

src/03-modality-agnostic-files.md

codecov · 2022-04-09T17:16:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.10%. Comparing base (e140f35) to head (29e9ca5).
Report is 1423 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1056      +/-   ##
==========================================
+ Coverage   70.53%   72.10%   +1.56%     
==========================================
  Files           9        9              
  Lines         930      950      +20     
==========================================
+ Hits          656      685      +29     
+ Misses        274      265       -9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sappelhoff

Okay, I got a new idea that I pushed in d0bea64

Basically:

whenever we specify a macro, we add an optional input relpath
relpath is a string containing the relative path to /src (the root of the spec)
E.g., for src/03-modality-agnostic-files.md, we have relpath="."
links in the schema are then specified as if directly relative to /src ... that is: [Units](/02-common-principles.html#units) becomes [Units](REPLACEME/02-common-principles.md#units)
the REPLACEME gets replaced with relpath during the macrocall

This does work locally ... not sure if it'll pass CI, because we'd also have to fix it in the glossary etc.

Consider this a proof of concept -- what are your thoughts?

EDIT: As expected, the CI fails due to warnings about the glossary (we run in strict mode, we warnings = failure). However, you can inspect the CircleCI artifacts, see: https://output.circle-artifacts.com/output/job/c904e3d6-4744-4642-8790-daf6eb0cbf57/artifacts/0/dev_docs/03-modality-agnostic-files.html#scans-file ... the first link to Units in the acq_time metadata is the link as it is currently ... the second one is using the fix described above.

sappelhoff · 2022-04-25T11:37:14Z

As evident from the last few commits I am just going ahead with my plan. I think it might also be easier to review if you see all the required changes in front of you, instead of thinking about it conceptually

sappelhoff

@VisLab you can see the build artifact of this PR and check that the links work again once the fixes here are applied: https://bids-specification--1056.org.readthedocs.build/en/1056/

effigies

This all seems reasonable. Some nit-picky questions/suggestions.

tools/schemacode/schemacode/render.py

src/02-common-principles.md

ericearl

@sappelhoff @effigies

If this works and looks good (I don't know how/where to look at the render, but I trust @sappelhoff has already viewed it), then I am in favor of putting it in there since it works.

I feel like the ideal solution just involves using python's os.walk() method looking for the referenced files "PATH_TO_SRC" automatically by exact basename (this assumes we don't have two files with the same names inside any of the .MD files). And pair that with the os.path.relpath() method to get the relative path within the repo from there. This would save anyone having to specify these extra "relpath" things that might be technical debt in the future.

sappelhoff

okay, the new way works and looks prettier and has lower maintenance burden (always using page.file instead of manually inserting a relative path).

BUT it doesn't work for the PDF build. I'll see if I can fix it.

sappelhoff · 2022-04-27T10:04:13Z

everything works now, ready for a new review. Thanks for the previous round -- I think it's better now.

ericearl

Great updates! Nice work and thanks.

effigies

Does the job. Small suggestions, and one probably ill-advised idea for extracting the filename in the macro functions, rather than repeatedly throughout the source.

effigies · 2022-04-27T16:49:23Z

tools/schemacode/schemacode/render.py

+        if page_file is not None:
+            relpath = os.sep.join([".."] * page_file.src_path.count(os.sep))
+            relpath = "." if not relpath else relpath
+            obj_desc = obj_desc.replace("SPEC_ROOT", relpath)


This could be refactored rather than repeated 4 times. It's also duplicating some stdlib behavior.

import posixpath def get_relpath(src_path): """ Retrieve relative path to the source root from the perspective of a Markdown file Examples -------- >>> get_relpath("02-common-principles.md") '.' >>> get_relpath("04-modality-specific-files/01-magnetic-resonance-imaging-data.md") '..' >>> get_relpath("we/lack/third_levels.md") '../..' """ return posixpath.relpath(".", posixpath.dirname(src_path))

Suggested change

if page_file is not None:

relpath = os.sep.join([".."] * page_file.src_path.count(os.sep))

relpath = "." if not relpath else relpath

obj_desc = obj_desc.replace("SPEC_ROOT", relpath)

if page_file is not None:

obj_desc = obj_desc.replace("SPEC_ROOT", get_relpath(page_file.src_path))

effigies · 2022-04-27T16:53:38Z

src/02-common-principles.md

@@ -486,7 +486,8 @@ and a guide for using macros can be found at
        "Units": "RECOMMENDED",
        "TermURL": "RECOMMENDED",
        "HED": "OPTIONAL",
-   }
+   },
+   page.file


I was kind of hoping just to make it implicit. It looks like you found where the information can be grabbed from, so if we're up for some deep magic of inspecting the call stack and extracting variables, I could make a PR.

Seems doable (#1096). Some cleanup needed if we do want to go that route.

Thanks for the proposal - I like it and commented there. If others agree I am happy to close this PR and let #1096 take over.

effigies · 2022-04-27T16:55:11Z

tools/schemacode/schemacode/tests/test_render.py

+
+
+test_file = TestFile()
+test_file.src_path = os.sep.join(["try", "this", "dict"])


Suggested change

test_file.src_path = os.sep.join(["try", "this", "dict"])

test_file.src_path = os.path.join("try", "this", "dict")

add test

a504596

sappelhoff commented Apr 9, 2022

View reviewed changes

src/03-modality-agnostic-files.md Show resolved Hide resolved

src/03-modality-agnostic-files.md Outdated Show resolved Hide resolved

try new method

d0bea64

sappelhoff commented Apr 9, 2022

View reviewed changes

tsalo added the schema Issues related to the YAML schema representation of the specification. Patch version release. label Apr 12, 2022

sappelhoff added 2 commits April 25, 2022 13:00

REPLACEME -> PATH_TO_SRC

cf75dd6

remove first attempt

dcda5eb

sappelhoff force-pushed the links/schema branch from 8b90267 to dcda5eb Compare April 25, 2022 11:04

sappelhoff added 5 commits April 25, 2022 13:15

add relpaths make_columns_table

758ac9f

use PATH_TO_SRC in columns.yaml

21ddd98

html -> md

1bf6426

add relpath arg for glossary

73b06a5

fix tests

754875a

add relpath for suffix tables

f04bfac

sappelhoff force-pushed the links/schema branch from f89aa5c to f04bfac Compare April 25, 2022 14:43

add relpath for metadata

e9ffe1e

sappelhoff marked this pull request as ready for review April 26, 2022 10:32

sappelhoff requested review from tsalo, effigies and chrisgorgo as code owners April 26, 2022 10:32

sappelhoff added 2 commits April 26, 2022 13:36

add tests

df82d4e

run black

823d9d3

sappelhoff mentioned this pull request Apr 26, 2022

Issue with documentation links being lost in translation to readthedocs #1095

Closed

sappelhoff commented Apr 26, 2022

View reviewed changes

effigies reviewed Apr 26, 2022

View reviewed changes

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

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

src/02-common-principles.md Outdated Show resolved Hide resolved

ericearl approved these changes Apr 26, 2022

View reviewed changes

sappelhoff added 2 commits April 27, 2022 10:12

PATH_TO_SRC -> SPEC_ROOT

f609994

fix param ordering consistency

3b3547b

sappelhoff added 6 commits April 27, 2022 10:59

'..'|'.' -> page.file

fe3dd0e

relpath -> page_file

e36c768

detect relpath from page.file

8f30d1e

fix docstrings

8c1f903

fix tests

f8d339c

run black

0ca0319

sappelhoff commented Apr 27, 2022

View reviewed changes

fix PDF build

29e9ca5

sappelhoff requested review from effigies and bendhouseart April 27, 2022 10:03

ericearl approved these changes Apr 27, 2022

View reviewed changes

effigies approved these changes Apr 27, 2022

View reviewed changes

effigies mentioned this pull request Apr 27, 2022

[INFRA] Fix internal links implicitly #1096

Merged

sappelhoff marked this pull request as draft April 29, 2022 06:39

sappelhoff closed this in #1096 Apr 29, 2022

sappelhoff mentioned this pull request Jun 16, 2022

URI link broken in BIDS microscopy #1129

Closed

sappelhoff deleted the links/schema branch July 20, 2022 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INFRA] fix spec-internal links from schema entries #1056

[INFRA] fix spec-internal links from schema entries #1056

sappelhoff commented Apr 9, 2022 •

edited

Loading

codecov bot commented Apr 9, 2022 •

edited

Loading

sappelhoff left a comment •

edited

Loading

sappelhoff commented Apr 25, 2022

sappelhoff left a comment

effigies left a comment

ericearl left a comment

sappelhoff left a comment

sappelhoff commented Apr 27, 2022

ericearl left a comment

effigies left a comment

effigies Apr 27, 2022 •

edited

Loading

effigies Apr 27, 2022

effigies Apr 27, 2022

sappelhoff Apr 27, 2022

effigies Apr 27, 2022



		test_file = TestFile()
		test_file.src_path = os.sep.join(["try", "this", "dict"])

	test_file.src_path = os.sep.join(["try", "this", "dict"])
	test_file.src_path = os.path.join("try", "this", "dict")

[INFRA] fix spec-internal links from schema entries #1056

[INFRA] fix spec-internal links from schema entries #1056

Conversation

sappelhoff commented Apr 9, 2022 • edited Loading

codecov bot commented Apr 9, 2022 • edited Loading

Codecov Report

sappelhoff left a comment • edited Loading

Choose a reason for hiding this comment

sappelhoff commented Apr 25, 2022

sappelhoff left a comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

ericearl left a comment

Choose a reason for hiding this comment

sappelhoff left a comment

Choose a reason for hiding this comment

sappelhoff commented Apr 27, 2022

ericearl left a comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

effigies Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

effigies Apr 27, 2022

Choose a reason for hiding this comment

effigies Apr 27, 2022

Choose a reason for hiding this comment

sappelhoff Apr 27, 2022

Choose a reason for hiding this comment

effigies Apr 27, 2022

Choose a reason for hiding this comment

sappelhoff commented Apr 9, 2022 •

edited

Loading

codecov bot commented Apr 9, 2022 •

edited

Loading

sappelhoff left a comment •

edited

Loading

effigies Apr 27, 2022 •

edited

Loading