Skip to content
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

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

Closed
jhlegarreta opened this issue May 2, 2021 · 5 comments · Fixed by #594 or #597
Closed

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

jhlegarreta opened this issue May 2, 2021 · 5 comments · Fixed by #594 or #597
Assignees
Labels
type:template and tools Issue about template and tools

Comments

@jhlegarreta
Copy link

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 maxim-belkin self-assigned this May 6, 2021
@maxim-belkin maxim-belkin added the type:template and tools Issue about template and tools label May 6, 2021
@maxim-belkin
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

maxim-belkin commented May 6, 2021

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)

maxim-belkin added a commit that referenced this issue May 6, 2021
bin/lesson_check.py: allow exceptions to line length limit

Fixes #591
@jhlegarreta
Copy link
Author

Thanks for these helpful explanations @maxim-belkin 💯.

@maxim-belkin
Copy link
Contributor

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

maxim-belkin added a commit that referenced this issue May 9, 2021
lesson_check.py: harden single-line image/link pattern

Fixes #591
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:template and tools Issue about template and tools
Projects
None yet
2 participants