Tests.QuickCheckUtils: refactors

[Anton-Latukha]

The main agenda of the patch is to refactor the code of Arbitrary {,L}Text generation.

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): refact a2a2179 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.

[Bodigrim]

@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.

[Anton-Latukha]

Could you possibly elaborate what is the issue you are solving in this PR?

I went to do subsequence tests & so generation, and so - looked into the generation of {,L}Text.

& 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 coerce, in a library as text starting that kind of a discussion can make a feedback splash. So it is good to test waters to not stir up the pot, again, this is a test suite module, so generally, people would not complain about some coerce there, while this module has half of newtypes, their instances & rewrapping in the project, so it is a central place to apply coerce in the package.

Because generally speaking I prefer something with specific types like fmap (NotEmpty . TL.pack . getNonEmpty) over fmap (coerce . f . coerce)

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 coerce then are largely the details.

coerce is a great addition to {pure, mempty} arsenal. It is one of the most powerful things I've seen to simplify the code, do transformations & navigate the type system, it is polymorphic, while also completely free. There is no longer a need to concentrate on either that is rewrapper or not and how that rewrapper is called for the type, un.*,get.*,run.*,..., or constructor, and how to compose that, coerce eliminates them, it can easilly transition between quite complex types, which is fully controlled by type-level code. And I love to read the needed type information, not from the constructors/accessor names (execution level code), but from the type signatures and type applications. And from now on GHC/HLS point out the type conversion for coerce. Some people say that from the 90s on, the introduction of a coerce is the biggest language change that happened, to the Haskell core languages for sure.

Completely Ok with some core Haskell project as text keeping the style. Because transition transforms the code by quite a bit, especially also because coerce goes together with ViewPatterns here and there, like even in this file - in the BigInt/Big constructor type in this file.

And already submitted a type error (using Gen [Sqrt (NotEmpty T.Text)] not Gen (Sqrt [NotEmpty T.Text])) while converting the code, so guilty as charged, one can make an error doing the transition.

[Anton-Latukha]

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 coerce in the computation modules.

[Bodigrim]

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.

[Bodigrim]

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 

[Anton-Latukha]

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".

[Bodigrim]

Could we somehow avoid undefined smell here and above?

[Anton-Latukha]

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.

[Lysxia]

The motivation for these stylistic changes is quite subjective, it doesn't seem strong enough to merge this PR.

[Anton-Latukha]

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.

[Bodigrim]

@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.

[Lysxia]

@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.

[Anton-Latukha]

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 coerce is not encouraged - lets just close this.

[Bodigrim]

@Anton-Latukha sorry for silence, I'm too busy to respond properly. From my perspective, more active use of coerce (especially when it leads to non-trivial runtime improvements) is fine, while other refactorings are better to be delayed.
CC @Lysxia @Boarders @parsonsmatt