-
Notifications
You must be signed in to change notification settings - Fork 3
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
[#64] Add annotations for the copypaste check #246
base: YuriRomanowski/#64-Implement-copy-paste-protection
Are you sure you want to change the base?
[#64] Add annotations for the copypaste check #246
Conversation
Problem: Currently xrefcheck is able to detect possibly bad copy-pastes, but there is no way to disable those checks locally for a file/paragraph/link. Solution: Add support for related annotations for `.md` files.
@@ -36,11 +36,13 @@ Unreleased | |||
+ Now we call references to anchors in current file (e.g. `[a](#b)`) as | |||
`file-local` references instead of calling them `current file` (which was ambiguous). | |||
* [#233](https://github.com/serokell/xrefcheck/pull/233) | |||
+ Now xrefxcheck does not follow redirect links by default. It fails for permanent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
CHANGES.md
Outdated
redirect responses (i.e. 301 and 308) and passes for temporary ones (i.e. 302, 303, 307). | ||
* [#231](https://github.com/serokell/xrefcheck/pull/231) | ||
+ Anchor analysis takes now into account the appropriate case-sensitivity depending on | ||
the configured Markdown flavour. | ||
* [240](https://github.com/serokell/xrefcheck/pull/240) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this number have a '#' prefix?
README.md
Outdated
followed by an <!-- xrefcheck: no duplication check in link --> [copypasted intentionally](https://good.link.uri/). | ||
``` | ||
* You can use a `<!-- xrefcheck: no duplication check in paragraph -->` annotation to disable copy-paste check in a paragraph. | ||
* You can use a `<!-- xrefcheck: no duplication check in file -->` annotation to disable copy-paste check within an entire file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should mention that this annotation is only allowed at the top of the file.
"no duplication check in paragraph" annotation, but found #{txt}|] | ||
UnrecognisedErr txt -> [int||Unrecognised option "#{txt}", perhaps you meant | ||
<"ignore link"|"ignore paragraph"|"ignore all"> | ||
or "no duplication check in <link|paragraph|file>"?|] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two kind of annotation suggestions look a bit different. Maybe would be better something like
"ignore <link|paragraph|all>"
or "no duplication check in <link|paragraph|file>"
or
<"ignore link"|"ignore paragraph"|"ignore all">
or <"no duplication check in link"|"no duplication check in paragraph"|"no duplication check in file">
src/Xrefcheck/Core.hs
Outdated
diffToFileInfo (FileInfoDiff refs anchors) = | ||
FileInfo (DList.toList refs) (DList.toList anchors) | ||
diffToFileInfo :: Bool -> FileInfoDiff -> FileInfo | ||
diffToFileInfo ignoreCpcInFile (FileInfoDiff refs anchors) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignoreCpcInFile
stands for copy paste check enabled or disabled? It is being used directly as the _fiCopyPasteCheck
field argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a typo
src/Xrefcheck/Scanners/Markdown.hs
Outdated
|
||
wrapTraverseNodeWithLinkExpectedForCpc :: | ||
ScannerM NodeCPC -> | ||
ScannerM NodeCPC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In multiline function type declarations, I think that the style should be
wrapTraverseNodeWithLinkExpectedForCpc
:: ScannerM NodeCPC
-> ScannerM NodeCPC
-- find copy/paste check annotations (ignore for paragraph and for link) | ||
-- and label nodes with a boolean meaning whether they should be | ||
-- copy/paste checked. | ||
processAnnotations :: FilePath -> C.Node -> Writer [ScanError] NodeCPC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this code seems to not scale pretty well (in LOC) with new kinds of annotations added. Also, as we have been discussing, it is difficult to reason about how they overlap or affect each other.
But it is such a big topic that we could leave it for another issue, in which we try to figure out some clear abstraction over Xrefcheck annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if any new annotations will behave the same as current ones, it may be not a big problem. But for different annotations yes, it won't be easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! I'm leaving a few comments.
Review: style, typos, naming
Description
Problem: Currently xrefcheck is able to detect possibly bad
copy-pastes, but there is no way to disable those checks
locally for a file/paragraph/link.
Solution: Add support for related annotations for
.md
files.Related issue(s)
Fixes #64
✅ 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
silently reappearing again.
Documentation
Public contracts
of Public Contracts policy.
and
Stylistic guide (mandatory)
✓ Release Checklist
package.yaml
.under the "Unreleased" section to a new section for this release version.
vX.Y.Z
.xrefcheck-action
.