hpc-carpentry / hpc-parallel-novice

Introductory material on parallelization using python with a focus on HPC platforms
https://hpc-carpentry.github.io/hpc-parallel-novice
Other
2 stars 5 forks source link

Sync upstream lesson_check #15

Closed tkphd closed 3 years ago

tkphd commented 3 years ago

carpentries/lesson-example updated lesson_check.py to accept any language-* code block, along with a few minor changes.

bkmgit commented 3 years ago

Thanks.

bkmgit commented 3 years ago

Running make lesson-check-all gives: bin/lesson_check.py:384: FutureWarning: Possible nested set at position 2 (not re.search(link, l)) and ./_episodes/05-bonus-mpi-for-pi.md: Line(s) too long: 38 ./design.md: Line(s) too long: 23, 39

bkmgit commented 3 years ago

Based on the issue https://github.com/jekyll/jekyll/issues/5267 it would be helpful to add vendor to _config.yml to enable local builds.

bkmgit commented 3 years ago

When previewing locally, the header title appears twice DoubleHeader

tkphd commented 3 years ago

@bkmgit wrote:

Running make lesson-check-all gives:

bin/lesson_check.py:384: FutureWarning: Possible nested set at position 2
(not re.search(link, l)) and
./_episodes/05-bonus-mpi-for-pi.md: Line(s) too long: 38
./design.md: Line(s) too long: 23, 39

An interesting warning about some of our custom code. Since this is not an error, we could let it slide; but it does refer to a customization of our own,

link = '^[[].+[]]:' # regex for [link-abbrv]: address
over = [i for (i, l, n) in self.lines if (n > MAX_LINE_LEN) and
                                      (not l.startswith('!')) and
                                      (not re.search(code, l)) and
                                      (not re.search(link, l)) and
                                      (not l.startswith('http'))]

Is anybody better-versed with regular expressions able to comment?

tkphd commented 3 years ago

@bkmgit wrote:

Based on the issue jekyll/jekyll#5267 it would be helpful to add "vendor" to _config.yml to enable local builds.

.vendor is already excluded.

The linked issue refers to running jekyll serve, which circumvents the Bundler framework (and which created the .vendor folder to begin with). Was an error thrown by make site or make serve? These call Jekyll through Bundler, ensuring the local configuration is recognized.

tkphd commented 3 years ago

@bkmgit wrote:

[coc]: https://docs.carpentries.org/topic_folders/policies/code-of-conduct.html
[coc-reporting]: https://docs.carpentries.org/topic_folders/policies/incident-reporting.html

are in links.md - why do they need to be included again in CODE_OF_CONDUCT.md?

I agree that this is an unfortunate repetition. Without making these definitions, make lesson-check throws an error:

./CODE_OF_CONDUCT.md: Internally-defined links may be missing definitions: "Carpentry Code of Conduct"=>"coc", "reporting guidelines"=>"coc-reporting"
make: *** [Makefile:134: lesson-check] Error 1
tkphd commented 3 years ago

@bkmgit wrote:

May consider using regex instead of re due to better unicode support.

Excellent suggestion for the upstream maintainers.

tkphd commented 3 years ago

@bkmgit wrote:

When previewing locally, the header title appears twice

Yeah, it does that, and I have no idea why. The double-header does not appear on the deployed site: this is restricted to local builds only.

Fixing this local-rendering bug is out-of-scope for this PR.

bkmgit commented 3 years ago

@bkmgit wrote:

Based on the issue jekyll/jekyll#5267 it would be helpful to add "vendor" to _config.yml to enable local builds.

.vendor is already excluded.

The linked issue refers to running jekyll serve, which circumvents the Bundler framework (and which created the .vendor folder to begin with). Was an error thrown by make site or make serve? These call Jekyll through Bundler, ensuring the local configuration is recognized.

Needed to add vendor explicitly for it to build, error is thrown by both make serve and make build, vendor is not always a hidden directory.

bkmgit commented 3 years ago

Not sure if this should be an upstream change though.

tkphd commented 3 years ago

Thanks @bkmgit, this is a good cross-platform catch!