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

[#242] No scan symlinks as md files #261

Merged
merged 2 commits into from
Jan 19, 2023
Merged

Conversation

aeqz
Copy link
Contributor

@aeqz aeqz commented Dec 30, 2022

Problem: when the repository contains a symlink to a markdown file, it is processed by xrefcheck as if it was the same markdown file but in the symlink's location. This leads to broken references and can be avoided because neither GitHub nor GitLab try to render symlinks as the file they point to.

Solution: consider symlinks as no scannable files. In the future, we will consider to include a new dedicated scanner for symlinks if it works.

Related issue(s)

Fixes #242
Relates #244

✅ Checklist for your Pull Request

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 force-pushed the aeqz/#242-no-scan-symlinks-as-md branch from ff20ef4 to b68920f Compare December 30, 2022 12:33
@aeqz
Copy link
Contributor Author

aeqz commented Dec 30, 2022

After several attempts on where to ignore symlinks, I have implemented the simplest one and we will complicate it in a following PR for #244 if necessary.

@aeqz aeqz force-pushed the aeqz/#242-no-scan-symlinks-as-md branch from b68920f to 609282a Compare December 30, 2022 16:45
This was referenced Dec 30, 2022
Copy link
Member

@Martoon-00 Martoon-00 left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me.

I extra checked the commit description, and see everything I wanted to see there 👍

Leaving just two comments.

src/Xrefcheck/System.hs Outdated Show resolved Hide resolved
@@ -44,10 +44,11 @@ Unreleased
* [#254](https://github.com/serokell/xrefcheck/pull/254)
+ Now the `dump-config` command does not overwrite a file unless explicitly told with a
`--force` flag. Also, a `--stdout` flag allows to print the config to stdout instead.
the configured Markdown flavour.
Copy link
Member

Choose a reason for hiding this comment

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

That's a good change 👍

And I would like it to be placed to a different commit 😸

Copy link
Member

Choose a reason for hiding this comment

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

And the addition below should probably belong to the main commit )

Also, I would add a description to that commit you extracted after all. At a first glance, it's not clear why exactly you delete this line, maybe it was meant to contain some important information and should be extended, not removed. If in the commit description you mention that this was an accidental copy-paste, that would make the reasoning much clearer for an arbitrary git history reader.

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 force-pushed the aeqz/#242-no-scan-symlinks-as-md branch from 609282a to 066da26 Compare January 12, 2023 19:16
@aeqz aeqz force-pushed the aeqz/#242-no-scan-symlinks-as-md branch from 066da26 to 1431587 Compare January 12, 2023 19:21
@aeqz aeqz requested a review from Martoon-00 January 12, 2023 20:14
@aeqz aeqz force-pushed the aeqz/#242-no-scan-symlinks-as-md branch from 1431587 to e45ebd8 Compare January 16, 2023 17:50
aeqz added 2 commits January 18, 2023 13:28
Problem: There is a duplicated line in the changelog unreleased section,
which seems to be an accidental copy-paste typo or a bad merge conflict
resolution.

Solution: Delete the aforementioned changelog line.
Problem: When the repository contains a symlink to a markdown file, it
is processed by xrefcheck as if it was the same markdown file but in the
symlink's location. This leads to broken references and can be avoided
because neither GitHub nor GitLab try to render symlinks as the file
they point to.

Solution: Consider symlinks as no scannable files. In the future, we
will consider to include a new dedicated scanner for symlinks if it
works.
@aeqz aeqz force-pushed the aeqz/#242-no-scan-symlinks-as-md branch from e45ebd8 to fef5153 Compare January 18, 2023 12:29
@aeqz aeqz merged commit ea64357 into master Jan 19, 2023
@aeqz aeqz deleted the aeqz/#242-no-scan-symlinks-as-md branch January 19, 2023 17:55
@int-index int-index mentioned this pull request Dec 27, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Some symlinks are scanned when not necessary
3 participants