carpentries / styles

Styles for The Carpentries lessons. No README to avoid merge conflicts with lessons. Demo 👇
https://carpentries.github.io/lesson-example
Other
86 stars 97 forks source link

Allow skipping line length checks for link-only lines #591

Closed jhlegarreta closed 3 years ago

jhlegarreta commented 3 years ago

The make leson-check-all script checks the line lengths of the Markdown files. If I'm not mistaken by the check results, lines containing only links dot not raise warnings if exceeding the allowed line length. However, such warnings are indeed raised for lines containing only links in the solution blocks.

Please correct me if I'm wrong, but I believe that links cannot be split across lines.

Below are some examples of such warnings: The GitHub badge links on file https://raw.githubusercontent.com/carpentries-incubator/SDC-BIDS-dMRI/main/README.md pointed in https://github.com/carpentries-incubator/SDC-BIDS-dMRI/runs/2438761253?check_suite_focus=true#step:16:19

Line 456 on file https://github.com/carpentries-incubator/SDC-BIDS-dMRI/blob/99dd1d1235e78bf940c4728974635fec5a00fabc/_episodes/constrained_spherical_deconvolution.md pointed in https://github.com/carpentries-incubator/SDC-BIDS-dMRI/runs/2438761253?check_suite_focus=true#step:16:20

Line 314 on file https://github.com/carpentries-incubator/SDC-BIDS-dMRI/blob/99dd1d1235e78bf940c4728974635fec5a00fabc/_episodes/deterministic_tractography.md pointed in https://github.com/carpentries-incubator/SDC-BIDS-dMRI/runs/2438761253?check_suite_focus=true#step:16:21

Thus, ideally the checker should ideally skip such lines/avoid raising warnings for such lines even in the solution block quotes (or any other relevant block quotes).

Thanks.

maxim-belkin commented 3 years ago

Thanks for opening the issue, @jhlegarreta. You're correct in that make lesson-check-all treats (long) lines differently depending on certain conditions. Currently, lines that contain images (not links) are allowed to go over the suggested line length limit (100 characters). I agree that it makes sense to allow lines that contain images and links only to go over the suggested line length limit (because it makes sense to keep URLs/paths on a single line). I'll submit a PR fixing that.

jhlegarreta commented 3 years ago

Thanks @maxim-belkin . In my issue, when mentioning links I meant images, i.e. lines like:

> > ![ODFs of differing crossing angles]({{ relative_root_path }}/fig/constrained_spherical_deconvolution/odf_multiple_angles.png){:class="img-responsive"} \
ODFs of different crossing angles.

in https://github.com/carpentries-incubator/SDC-BIDS-dMRI/blob/99dd1d1235e78bf940c4728974635fec5a00fabc/_episodes/constrained_spherical_deconvolution.md

or

> > ![Binary Stopping Criterion Tractography]({{ relative_root_path }}/fig/deterministic_tractography/tractogram_deterministic_ex1.png){:class="img-responsive"}

in https://github.com/carpentries-incubator/SDC-BIDS-dMRI/blob/99dd1d1235e78bf940c4728974635fec5a00fabc/_episodes/deterministic_tractography.md

but the way such lines are built in our case is maybe not correctly handled by the current implementation.

But yes, the comment would ideally apply to any kind of link.

maxim-belkin commented 3 years ago

I submitted #594 that should allow these exceptions. Note, that if desired you can go under the line length limit even now using...

~internal~ reference links:

![ODFs of differing crossing angles][image01]{:class="img-responsive"}

[image01]: {{ relative_root_path }}/fig/constrained_spherical_deconvolution/odf_multiple_angles.png

and some Liquid magic for very long alt. texts:

{% capture alt_text01 %}
My very long
alt text
{% endcapture %}

![{{alt_text01 | strip_newlines}}][image01]{:class="img-responsive"}

[image01]: {{ relative_root_path }}/fig/constrained_spherical_deconvolution/odf_multiple_angles.png

Technically, you can go even further ~down the rabbit hole~:

{% capture alt_text01 %}My very long alt text{% endcapture %}
{% assign pfig = relative_root_path | append: "/fig/constrained_spherical_deconvolution" %}
![{{alt_text01 | strip_newlines}}][image01]{:class="img-responsive"}
[image01]: {{ pfig }}/odf_multiple_angles.png

but, as you can see, it's a very small improvement (the longest line is still pretty long)

jhlegarreta commented 3 years ago

Thanks for these helpful explanations @maxim-belkin :100:.

maxim-belkin commented 3 years ago

@jhlegarreta, I'm reopening this issue since #594 hasn't fixed the issue completely for you. I'll comment about why this happened there.