-
Notifications
You must be signed in to change notification settings - Fork 4
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
[#239][#249] Further filepath refactor #263
Conversation
Generated by 🚫 Danger |
How should I properly write the PR and commit issue id referring to two different issues? I have tackled both of them at the same time because, when trying to refine when POSIX links are used and when platform dependent ones, I realised that we just need relative POSIX links during the verification phase (either relative to the repository root or to other repository file) and system dependent ones during the scan phase, which do not require further processing. |
Yeah, sounds fair. Usually we write e.g. |
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.
I reviewed superficially for now, and quite like how it looks like.
And otherall to me this looks simpler and more robust now.
@YuriRomanowski I would also appreciate your review, if you find time; meanwhile I'll try to spend more time on your PRs.
57942fd
to
dde6f9d
Compare
src/Xrefcheck/System.hs
Outdated
|
||
-- | Join two 'CanonicalRelPosixLink's. | ||
(</>) :: RelPosixLink -> RelPosixLink -> RelPosixLink | ||
UnsafeRelPosixLink a </> UnsafeRelPosixLink b = |
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.
This seems quite arbitrary but, as now RelPosixLink
s correspond to the original ones, with no normalisation applied, the output was displaying paths like "././file.md"
(and this would be the same even by using </>
from System.FilePath
, I just changed it to avoid String
<-> Text
conversions):
ghci> import System.FilePath
ghci> "." </> "./file.md"
"././file.md"
I will undo that change. As you say, it was already consistent. Now I wonder if counting "other" references as local progress could be better initialised by using lenses: vrLocal = initProgress $
(length $ references ^.. folded . to rInfo . _RIFile) +
(length $ references ^.. folded . to rInfo . _RIExternal . _ELOther) This is not valid, but something similar would be cool instead: vrLocal = initProgress $ length $
references ^.. folded . to rInfo . (_RIFile <|> _RIExternal . _ELOther) |
96befba
to
0a72726
Compare
Thanks 🙏
Oh, I cannot miss this call for perfection! AFAIR internally Plus This works for me: > let vals = [ Left "a", Right (Left 0), Right (Right 1) ] :: [Either String (Either Word Int)]
> vals ^.. L.folded . (L._Left . L.united <> L._Right . L._Right . L.united)
[(),()] |
Nice! I was also trying to unify types, but to Your proposal is simpler and worked perfectly 👌 |
e879454
to
a0cd95b
Compare
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.
One more comment
327f2bb
to
446b52b
Compare
aa1818e
to
8013bf7
Compare
I would be worried about some points anyways:
|
a8428f8
to
4f96c10
Compare
That's true, I forgot that tests are run not only by CIs. Hmm, if I remember correctly, we already had tests that imply different output under Linux and Windows. That test is written as |
I tried to add my example and the Windows CI failed: the repo cannot be cloned if it contains the |
The last request: do we have tests on paths case-sensitivity? That I think that would be aa finishing touch for this refactoring. |
We have tests for anchor case-sensitivity, but not for path case-sensitivity. I will add them 👍 |
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.
Great, thank you for this work!
Please squash the fixup commits and let's merge.
Problem: After refactoring the FilePath usages in the codebase to have a canonical representation of them, we noticed that further improvements could be applied, such as clarifying whether the path is system dependent and avoiding absolute file system paths. Solution: We now use POSIX relative paths during the analysis, and system dependent ones for reading file contents in the scan phase.
Problem: After changing the progress bar interface to require progress unit witnesses, some functions related to an anonymous task timestamp also started to require a progress unit witness, which complicates its usage unnecessarily. Solution: Do not require a progress unit witness for getTaskTimestamp.
Problem: When a file name contains unusual characters, its output line in git ls-files is quoted and Xrefcheck parses the file name wrong. Solution: Use the git ls-files -z option to output lines verbatim with null character line terminators. The file not found message has also been improved for the case in which its link contains a backslash.
c4a4827
to
236a006
Compare
Description
Problem: after refactoring the
FilePath
usages in the codebase to have a canonical representation of them, we noticed that further improvements could be applied, such as clarifying whether the path is system dependent and avoiding absolute file system paths.Solution: we now use POSIX relative paths during the analysis, and system dependant ones for reading file contents in the scan phase.
Related issue(s)
Fixes #239
Fixes #249
Relates #219
✅ Checklist for your Pull Request
Related changes
Tests
silently reappearing again.
Documentation
Public contracts
of Public Contracts policy.
and
Stylistic guide