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.

Sign up for GitHub

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

Jump to bottom

[#211] Case insensitive anchors #231

Merged
merged 3 commits into from
Dec 13, 2022
Merged

[#211] Case insensitive anchors #231

merged 3 commits into from
Dec 13, 2022

Conversation

aeqz
Copy link
Contributor

@aeqz aeqz commented Dec 5, 2022 •
edited
Loading

Description

Problem: Some Markdown flavours such as the GitHub one are case insensitive regarding anchors, but our analysis is currently case sensitive and it produces false positives.

Solution: Support case-insensitivity depending on the configured Markdown flavour. Apply this also to ambiguous and similar anchors detection.

Related issue(s)

Fixes #211

✅ Checklist for your Pull Request

Ideally a PR has all of the checkmarks set.

If something in this list is irrelevant to your PR, you should still set this
checkmark indicating that you are sure it is dealt with (be that by irrelevance).

Related changes (conditional)

  • Tests

    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from
      silently reappearing again.

  • Documentation

    • I checked whether I should update the docs and did so if necessary:

  • Public contracts

    • Any modifications of public contracts comply with the Evolution
      of Public Contracts       policy.
    • I added an entry to the changelog if my changes are visible to the users
      and
    • provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

aeqz
aeqz commented Dec 5, 2022
View reviewed changes
package.yaml
@@ -65,6 +65,7 @@ ghc-options:
- -Wno-all-missed-specialisations
- -Wno-prepositive-qualified-module
- -Wno-monomorphism-restriction
- -optP-Wno-nonportable-include-path
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In macOS, the project builds successfully but it shows an error message. It seems to correspond to a currently open issue for cabal, and adding this GHC option is a workaround for avoiding the message:

haskell/cabal#4739

Is it ok if I include it in this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds fine.

Please leave a brief comment mentioning what this option achieves, and I would also like to see it in a separate commit as it is unrelated to the main logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Is this comment style appropriate for a stack file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

A minor suggestion: it may be beneficial to also include the link where that solution was suggested, but it's really optional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good, thank you.

aeqz
aeqz commented Dec 5, 2022
View reviewed changes
tests/golden/check-git/check-git.bats
@@ -12,6 +12,7 @@ load '../helpers'
@test "Git: not a repo" {
cd $TEST_TEMP_DIR

export LANG=en_US
run xrefcheck

assert_output --partial "fatal: not a git repository"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Force the git error message to be in English, so this test works also if anyone has other language configured.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good 👍

Now please extract this change to a separate commit, so that someone viewing the commits history could see the motivation for this change. Feel free to put that new commit as the oldest, if this simplifies further work for you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, in that new commit, put description in the Problem/Solution format.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, thank you 👍

@aeqz aeqz requested a review from YuriRomanowski December 7, 2022 14:58
@aeqz aeqz force-pushed the aeqz/#211-case_insensitive_anchors branch from 5b613dc to ba67d87 Compare December 8, 2022 15:31
Martoon-00
Martoon-00 reviewed Dec 8, 2022
View reviewed changes
tests/golden/check-git/check-git.bats
@@ -12,6 +12,7 @@ load '../helpers'
@test "Git: not a repo" {
cd $TEST_TEMP_DIR

export LANG=en_US
run xrefcheck

assert_output --partial "fatal: not a git repository"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good 👍

Now please extract this change to a separate commit, so that someone viewing the commits history could see the motivation for this change. Feel free to put that new commit as the oldest, if this simplifies further work for you.

@aeqz aeqz force-pushed the aeqz/#211-case_insensitive_anchors branch from ba67d87 to 3aa4cef Compare December 12, 2022 09:46
@aeqz aeqz requested a review from Martoon-00 December 12, 2022 11:14
@aeqz aeqz force-pushed the aeqz/#211-case_insensitive_anchors branch from d5abd72 to 00cb9dd Compare December 12, 2022 16:20
aeqz added 2 commits December 13, 2022 10:20
@aeqz
[#211] Case insensitive anchors
e8d79e7 
Problem: Some Markdown flavours such as the GitHub one are case
insensitive regarding anchors, but our analysis is currently
case sensitive and it produces false positives.

Solution: Support case-insensitivity depending on the configured
Markdown flavour. Apply this also to ambiguous and similar anchors
detection.
@aeqz
[#211] Specify language in a golden test
7457a6b 
Problem: We have a Golden test that expects an output in English
and fails if a different language is configured.

Solution: Configure explicitly the language before running the
corresponding test.
@aeqz aeqz force-pushed the aeqz/#211-case_insensitive_anchors branch from 00cb9dd to 4763802 Compare December 13, 2022 09:23
@aeqz
[#211] Avoid warning on case-insensitive systems
dd52970 
Problem: There is currently some problem in stack or cabal
that produces a warning when building this project on
case-insensitive systems.

Solution: The current workaroud for it is to add the GHC
option '-optP-Wno-nonportable-include-path'.
@aeqz aeqz force-pushed the aeqz/#211-case_insensitive_anchors branch from 4763802 to dd52970 Compare December 13, 2022 12:00
@aeqz aeqz merged commit 50e4e3b into master Dec 13, 2022
@aeqz aeqz deleted the aeqz/#211-case_insensitive_anchors branch December 13, 2022 12:23
@int-index int-index mentioned this pull request Dec 27, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Martoon-00 Martoon-00 Martoon-00 approved these changes

@YuriRomanowski YuriRomanowski YuriRomanowski approved these changes

@dcastro dcastro Awaiting requested review from dcastro

@Sorokin-Anton Sorokin-Anton Awaiting requested review from Sorokin-Anton

@Sereja313 Sereja313 Awaiting requested review from Sereja313

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow uppercase in anchors
3 participants
@aeqz @Martoon-00 @YuriRomanowski