bin/lesson_check.py: allow comments and empty lines in links.md

[maxim-belkin]

Fixes #581

@jhlegarreta, this PR should allow empty lines and single-line HTML comments in links.md

[jhlegarreta]

Looks good.

Just for cross-checking, and following the inconsistent behavior observed in #581 (comment), it might be good if you could try, as time permit, editing the links.md in this repository and seeing if the issue is reproducible or whether it was a glitch.

Also, if it happens to be reproducible, it might be good to add checks for other Markdown-syntax comments.

Thanks for the effort.

[maxim-belkin]

it might be good if you could try, as time permit, editing the links.md in this repository and seeing if the issue is reproducible or whether it was a glitch.

Failing on (single-line) HTML comments:
https://github.com/maxim-belkin/python-novice-inflammation/runs/2423552360?check_suite_focus=true#step:14:7

[jhlegarreta]

Thanks for the update and effort @maxim-belkin 💯.

Yesterday I run the checks on the https://github.com/carpentries-incubator/SDC-BIDS-dMRI carpentries incubator repository locally multiple times and no failures were reported concerning the links.md file across such runs. So now I am unable to reproduce the failure, as anticipated in #581 (comment). I am at a loss.

But please go ahead and merge if you are convinced that the issue I opened is still relevant and that this addresses it. Maybe you will want to consider other Markdown-allowed comment mark-ups.

[maxim-belkin]

Yesterday I run the checks on the https://github.com/carpentries-incubator/SDC-BIDS-dMRI carpentries incubator repository locally multiple times and no failures were reported concerning the links.md file across such runs.

This check is skipped for the lessons that use remote theme (and your lesson uses one) assuming that these lessons use the links.md file from the remote theme:

styles/bin/lesson_check.py

Lines 118 to 119 in 2abba21

	if not using_remote_theme(args.source_dir):
	args.references = read_references(args.reporter, args.reference_path)

Perhaps instead of skipping this check for such lessons, we should check if the lesson provides the file and test it.

I see assests folder in your repo. I don't know if you kept it on purpose, but the instructions I found for lessons that use remote theme say to delete it: https://github.com/carpentries/carpentries-theme/blob/main/USAGE.md

[jhlegarreta]

Perhaps instead of skipping this check for such lessons, we should check if the lesson provides the file and test it.

Sounds reasonable. However, following your advice we split the links.md file to use a separate one for the lesson-specific links, and the proposed solution should then try to identify lesson-specific links files (or propose a naming pattern to ease that identification) so that they can get tested (assuming links.md gets tested upstream).

I see assests folder in your repo. I don't know if you kept it on purpose, but the instructions I found for lessons that use remote theme say to delete it: https://github.com/carpentries/carpentries-theme/blob/main/USAGE.md

Thanks for letting us know. I have inspected the file, and it does not contain any lesson-specific changes, so I'd dare to say that it was downloaded when sync'ing with the upstream styles repository. So probably downloading the file should be skipped when sync'ing. Thus, note sure if following this the file is skipped: https://github.com/carpentries/maintainer-resources/blob/main/FLIGHT_RULES.md#update-styles.

Thanks for the effort.

[maxim-belkin]

@zkamvar @fmichonneau, do you have any questions or concerns?

[maxim-belkin]

Thanks for approving, @fmichonneau!