Tests.QuickCheckUtils: refactors #374
Conversation
There was a problem hiding this comment.
@Anton-Latukha thanks! Could you possibly elaborate what is the issue you are solving in this PR? Because generally speaking I prefer something with specific types like fmap (NotEmpty . TL.pack . getNonEmpty) over fmap (coerce . f . coerce). While there are performance reasons to use coerce instead of fmap getNonEmpty, on other occasions I'd prefer to keep concrete constructors/accessors.
I went to do subsequence tests & so generation, and so - looked into the generation of & I love refactoring, a good did, while helps me understand the code and make reading & working with it easier for others. It is in large a try of how the dialog would go, without opening a report. It is a test suite module, so it is once removed from the active changes. Because I generally do not want to open a report/discussion/code style discussions in core packages, especially on the use of
Well - that is the thing I was testing. Since I think that the active maintainer determines the code style in the project. I personally love supplying type signatures to all (even local) functions, so when some change happens - having a solid typing suite to work with, and
Completely Ok with some core Haskell project as And already submitted a type error (using |
Anton-Latukha
force-pushed
the
2021-09-30-refactors
branch
2 times, most recently
from
fcbbf1e to
44af64c
Compare
|
So, make a decision, I would comply with it, I can drop any of the commits made. And as I understood, would know not to refactor with |
There was a problem hiding this comment.
Strictly speaking, I'd like to discourage refactoring for the sake of refactoring, without specific tangible benefits. I also strongly suggest to raise an issue first and get at least one of maintainers on board before preparing a PR (unless it is a trivial typo or life cycling).
But since we already spent time to review, let's make a one-off exception and grasp a possibility to polish this module even further.
tests/Tests/QuickCheckUtils.hs
Outdated
| shrink = map Sqrt . shrink . unSqrt | ||
| arbitrary = coerce $ sized $ \n -> resize (smallish n) $ arbitrary @a | ||
| where | ||
| smallish = round . (sqrt :: Double -> Double) . fromIntegral . abs |
There was a problem hiding this comment.
This seems to be a perfect use case for @phadej's approximate square root. Note that one can avoid division and remain entirely in the realm of bit fiddling by something along the following lines:
intSqrt :: Int -> Int
intSqrt n = if n < 2 then n else let b2 = shiftR (finiteBitSize n - countLeadingZeros n) 1 in shiftR (shiftL 1 b2 + shiftR n b2) 1 There was a problem hiding this comment.
Yes. Binary division can be replaced with bit shifting.
First, I should understand and test this particular implementation myself, before putting it into the code.
But it is a place where it starts to feel as overengineering. My work was mainly to iso the code improving code quality in elegance/readability, so that the code is easier to work with, some performance gains are a byproduct of that. To do a custom code for RAM bit shifting to optimize testsuite times seems a bit too much.
But a nice idea overall. If intSqrt is really needed, to do it properly, it should be included into libraries. A nice thing to contribute upstream.
Of course, you have https://hackage.haskell.org/package/integer-roots-1.0.0.1/docs/src/Math.NumberTheory.Roots.Squares.html#integerSquareRoot.
So far I not done this. So now PR-review status goes to "not finished".
tests/Tests/QuickCheckUtils.hs
Outdated
|
|
||
| instance Arbitrary (Precision Double) where | ||
| arbitrary = arbitraryPrecision 22 | ||
| shrink = map Precision . shrink . precision undefined | ||
| shrink = coerce . shrink . precision undefined |
There was a problem hiding this comment.
Could we somehow avoid undefined smell here and above?
There was a problem hiding this comment.
precision had a redundant type witness, it was just yet another manual implementation of coerce.
As function is exported - instead of removing, placed a deprecation message and a date.
Anton-Latukha
force-pushed
the
2021-09-30-refactors
branch
from
44af64c to
7a7b301
Compare
|
The motivation for these stylistic changes is quite subjective, it doesn't seem strong enough to merge this PR. |
@Lysxia It is anti-social to try to block the PRs of new contributors. Best practices in the programming community are formulated quite nicely. You do your work, I do mine. We did mistakes establishing a dialog in #369 (comment). I personally do not have any problem with you nor with your positions. |
|
@Anton-Latukha your reaction to #374 (comment) is out ot proportion. @Lysxia is entitled to have an opinion and expressed it in the most neutral form. Being a new contributor does not make your PR immune to potential rejection. If you are unhappy with this approach (e. g., you find it detrimental for community growth), please raise the matter directly with CLC. As I already said in #374 (review), refactoring for the sake of refactoring is unwelcome. Indeed, as a gesture to a new contributor, I (as one of several maintainers) am still open to consider this PR. However, doubling down with stylistic changes as in 8f6ed3b will not bring us anywhere. |
|
@Anton-Latukha It is unfortunate that we started with two disagreements, but I have nothing against you personally either. It is my job to vet changes, and I voiced my opinion early and firmly so that other maintainers can react if they disagree, and otherwise to incite you to redirect your efforts on another issue if you're looking to make a contribution. |
|
Ok. The layouting in the module was highly inconsistent, since I looked at it & it seemed completely random mix - started to do mine one, pretty simple diagonal layouting. But ironically for refactoring. I would roll it back But if use of |
|
@Anton-Latukha sorry for silence, I'm too busy to respond properly. From my perspective, more active use of |
Anton-Latukha
force-pushed
the
2021-09-30-refactors
branch
2 times, most recently
from
a7fbc36 to
594d777
Compare
Anton-Latukha
force-pushed
the
2021-09-30-refactors
branch
from
594d777 to
1fdf592
Compare
The main agenda of the patch is to refactor the code of
Arbitrary {,L}Textgeneration.Then, because there were a lot of functoring of newtype accessors, went over a module and applied
coerce.Tests.QuickCheckUtils: i Arbitrary (NotEmpty {,L}Text): refacta2a2179 is put separately, since: generally prefer to already organize code used 2 times, but here it is a bit excessive pyramid for what that code is doing.