Refactor data collection. #71

Heimdell · 2021-07-29T13:35:05Z

Description

Problem: data collection is a function that calls itself 5 screens below the place it starts and is a pile of different, but unrelated actions, performed simultaneously.

Solution: divide the pile into parts and then combine parts externally.

Concrete changes

The xrefcheck: ignore X functionality is implemented as a separate function.
The data collection is a fold over the tree, and other reducers can be connected.

Results

Anchor/Reference colelctor fits into one screen now.

The Visitor API

foldNode :: (Monoid a, Monad m) => (Node -> m a) -> Node -> m a

Runs the provided action, collects the results.

Related issue(s)

None, preparing for #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
- 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:
  - README
  - Haddock
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)

My commits comply with the policy used in Serokell.
My code complies with the style guide.

worm2fed

Great PR description! but so dirty commit history..
Can you do something with it? It's become really hard to review your changes.

Go through your commits and decide which small steps to your final result this commits lead you.
Add description for each commit (as described in our policy) so we can review this steps separately and understand it.
Use git commit --fixup HASH to fix smth in your previous commits, so we can understand to which commits related your fixes.

src/Xrefcheck/Scanners/Markdown.hs

exec/Main.hs

src/Xrefcheck/Scanners/Markdown/ErrorMessage.hs

tests/Main.hs

Martoon-00 · 2021-07-29T15:00:10Z

src/Xrefcheck/Scanners/Markdown/ErrorMessage.hs

+  , "\"" <> txt <> "\""
+  , "perhaps you meant \
+      \<\"ignore link\"|\"ignore paragraph\"|\"ignore file\"> "
+  ]


I think this is not a good thing actually. Putting error messages far from the use site, we encourage their divergence from the actual problem (e.g. the actual error can obtain one more condition when it fires, but it is now easy to forget to update the error message respectively).

I think error messages and where they are used should both fit into one screen, maybe put in where/letl block if found to make the code less readable.

Martoon-00

My opinion, per change:

Using post-imports - kaef (though I wonder, didn't @worm2fed implemented this already somewhere else, taking Add requirement for qualified imports style#11 into account?)
1.5. Mixins removal - good; import Universum addition - ok.
Using polysemy - I would retract from doing this change until it actually appears necessary, because using it makes things more complex for newcomers, and there might be cases not well supported by it (as with any advanced framework).
Using the Visitor pattern - the same thing, the current Markdown scanner already does most of the things we ever want from it and it just works. After the rewrite, we use only inside event, so things just got more complex without immediate benefit. And it's not clear whether future changes in the scanner logic, if any, will fit into the current model without increasing its complexity in directions other than that you support now.
Error messages extraction to a separate module - responded in another comment. The fact of extraction is good, but error message and condition for it to fire should locate nearby IMO (they didn't before, but putting to a separate module sounds too harsh).
Global -wno-orphans - this sounds like a coolstory, I think orhpans are not a very good pattern so discouraging it with a warning is actually good. Or not? Also I heard that compilation time slightly increases with each module that allows orphan instances.
Trailing -- in haddock - why not just use the same style as in the remainder of the project :pepe_stare: If everyone comes with its own preferred style (that also probably changes every several years), that would be a mess.
The separation of ignored removal and info gathering itself - really nice to obtain it 👍
Moving functions from where to top-level - neat.

Well, quite a wall of comments, but this PR is sorta big too )

I would extract 1 & 1.5, maybe 4 (changed), 7 and 8 to a separate PR and leave other changes pending for now.

Martoon-00 · 2021-08-05T17:52:52Z

src/Xrefcheck/Core.hs

-instance Default FileInfo where
-  def = FileInfo [] []
+instance Semigroup FileInfo where
+  FileInfo a b <> FileInfo c d = FileInfo (a <> c) (b <> d)


And here we get O(n^2) for free :nabros:

Btw, it seems to me that having a builder version for some list-like type is quite a common practice, can it be generalized and put to some library 🤔

Martoon-00 · 2021-08-05T18:06:58Z

src/Xrefcheck/Scanners/Markdown/Node.hs

+--
+--   (Why isn't `Node` a type alias for `Data.Tree.Node`?)
+--
+instance {-# overlapping #-} Show DebugNode where


Why is it overlapping?

Martoon-00 · 2021-08-05T20:44:55Z

tests/Test/Xrefcheck/IgnoreAnnotationsSpec.hs

 import Test.Hspec (Spec, describe, it, shouldBe)

 import Xrefcheck.Core
 import Xrefcheck.Scanners.Markdown

-spec :: Spec
+spec :: HasCallStack => Spec


Is this constraint necessary here?

Martoon-00 · 2021-08-05T20:49:39Z

src/Xrefcheck/Verify.hs

+      |+ "' diverges with its label '" +| build txt
+      |+ "',\n   which might denote copy-paste that ended with either anchor\n\
+             \   or label left not updated\n\n"
+


This apparently was intended for another PR )

worm2fed · 2021-08-06T06:15:28Z

@Martoon-00

Using post-imports - kaef (though I wonder, didn't @worm2fed implemented this already somewhere else, taking Add requirement for qualified imports style#11 into account?)

We didn't merge it yet. I'll update my PR soon!)

Martoon-00 · 2021-08-07T15:52:07Z

@Martoon-00

Using post-imports - kaef (though I wonder, didn't @worm2fed implemented this already somewhere else, taking Add requirement for qualified imports style#11 into account?)

We didn't merge it yet. I'll update my PR soon!)

👍

Just coordinate with @Heimdell who switches this entire project to post-qualified-imports.

Problem: The tree traversal uses explicit recursion and does not-closely-unrelated stuff at once. Solution: Separate different actions.

src/Xrefcheck/Core.hs

src/Xrefcheck/Scanners/Markdown.hs

package.yaml

src/Xrefcheck/Scanners/Markdown.hs

Problem: Stack cannot build projects with mixins, and they only used to splice universum instead of base. Solution: Use `NoImplicitPreduce` and import `Universum` everywhere explicitly.

Problem: In ``` import qualified Foo.Bar as Bar import Foo.Bar (Bar) ``` names of the imported modules are on different vertical lines, which disables autosorting, and makes it harder to read. Solution: Use `ImportQualifiedPost`

Problem: The tree traversal uses explicit recursion and does not-closely-unrelated stuff at once. Solution: Separate different actions.

src/Xrefcheck/Scanners/Markdown.hs

Co-authored-by: Diogo Castro <[email protected]>

dcastro · 2021-11-09T10:56:48Z

@Martoon-00 do you wanna have another look at this before merging, or are you happy enough with merging as-is?

Heimdell force-pushed the slowpnir/copy-paste-in-lists branch from beef978 to 365e9d6 Compare July 29, 2021 13:45

Heimdell requested review from alyoanton9, Martoon-00, worm2fed and dcastro July 29, 2021 13:54

worm2fed suggested changes Jul 29, 2021

View reviewed changes

Martoon-00 reviewed Jul 29, 2021

View reviewed changes

Martoon-00 reviewed Aug 5, 2021

View reviewed changes

Heimdell force-pushed the slowpnir/copy-paste-in-lists branch from 8ed8f3c to fae3d97 Compare October 4, 2021 10:53

Heimdell added a commit that referenced this pull request Oct 20, 2021

[#71] Separate concerns in Node traversal.

8fad026

Problem: The tree traversal uses explicit recursion and does not-closely-unrelated stuff at once. Solution: Separate different actions.

Heimdell force-pushed the slowpnir/copy-paste-in-lists branch from fae3d97 to 8fad026 Compare October 20, 2021 11:53

Heimdell requested review from Martoon-00 and worm2fed October 20, 2021 12:01

worm2fed reviewed Oct 20, 2021

View reviewed changes

dcastro reviewed Oct 29, 2021

View reviewed changes

package.yaml Show resolved Hide resolved

package.yaml Show resolved Hide resolved

package.yaml Show resolved Hide resolved

dcastro reviewed Nov 1, 2021

View reviewed changes

src/Xrefcheck/Scanners/Markdown.hs Outdated Show resolved Hide resolved

src/Xrefcheck/Scanners/Markdown.hs Show resolved Hide resolved

src/Xrefcheck/Scanners/Markdown.hs Show resolved Hide resolved

Heimdell added 3 commits November 1, 2021 15:24

Remove mixins

2f9a9d8

Problem: Stack cannot build projects with mixins, and they only used to splice universum instead of base. Solution: Use `NoImplicitPreduce` and import `Universum` everywhere explicitly.

Improve readability of imports

92c3de5

Problem: In ``` import qualified Foo.Bar as Bar import Foo.Bar (Bar) ``` names of the imported modules are on different vertical lines, which disables autosorting, and makes it harder to read. Solution: Use `ImportQualifiedPost`

[#71] Separate concerns in Node traversal.

d644a95

Problem: The tree traversal uses explicit recursion and does not-closely-unrelated stuff at once. Solution: Separate different actions.

dcastro reviewed Nov 1, 2021

View reviewed changes

src/Xrefcheck/Scanners/Markdown.hs Show resolved Hide resolved

src/Xrefcheck/Scanners/Markdown.hs Outdated Show resolved Hide resolved

Update src/Xrefcheck/Scanners/Markdown.hs

420aee0

Co-authored-by: Diogo Castro <[email protected]>

Heimdell mentioned this pull request Nov 3, 2021

Make using DerivingStrategies a default #96

Closed

Heimdell added 2 commits November 4, 2021 18:14

Addressing requested changes

72a1b23

Add regression tests for ignore link bug

878775c

Heimdell force-pushed the slowpnir/copy-paste-in-lists branch from 6269a80 to 878775c Compare November 4, 2021 14:20

Heimdell requested review from dcastro and worm2fed November 4, 2021 14:20

Silence a warning

7e954c2

dcastro approved these changes Nov 5, 2021

View reviewed changes

Heimdell merged commit 7e954c2 into master Nov 9, 2021

delete-merged-branch bot deleted the slowpnir/copy-paste-in-lists branch November 9, 2021 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor data collection. #71

Refactor data collection. #71

Heimdell commented Jul 29, 2021 •

edited

Loading

worm2fed left a comment

Martoon-00 Jul 29, 2021 •

edited

Loading

Martoon-00 left a comment

Martoon-00 Aug 5, 2021

Martoon-00 Aug 5, 2021

Martoon-00 Aug 5, 2021

Martoon-00 Aug 5, 2021

worm2fed commented Aug 6, 2021

Martoon-00 commented Aug 7, 2021

dcastro commented Nov 9, 2021

Refactor data collection. #71

Refactor data collection. #71

Conversation

Heimdell commented Jul 29, 2021 • edited Loading

Description

Concrete changes

Results

The Visitor API

Related issue(s)

✅ Checklist for your Pull Request

Related changes (conditional)

Stylistic guide (mandatory)

worm2fed left a comment

Choose a reason for hiding this comment

Martoon-00 Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Martoon-00 left a comment

Choose a reason for hiding this comment

Martoon-00 Aug 5, 2021

Choose a reason for hiding this comment

Martoon-00 Aug 5, 2021

Choose a reason for hiding this comment

Martoon-00 Aug 5, 2021

Choose a reason for hiding this comment

Martoon-00 Aug 5, 2021

Choose a reason for hiding this comment

worm2fed commented Aug 6, 2021

Martoon-00 commented Aug 7, 2021

dcastro commented Nov 9, 2021

Heimdell commented Jul 29, 2021 •

edited

Loading

Martoon-00 Jul 29, 2021 •

edited

Loading