text
text copied to clipboard
haskell
Tests.QuickCheckUtils: refactors
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.
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)overfmap (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.
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.
The motivation for these stylistic changes is quite subjective, it doesn't seem strong enough to merge this PR.
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 https://github.com/haskell/text/pull/369#discussion_r719433214. I personally do not have any problem with you nor with your positions.
@Anton-Latukha your reaction to https://github.com/haskell/text/pull/374#issuecomment-934526031 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 https://github.com/haskell/text/pull/374#pullrequestreview-770601317, 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 8f6ed3b361412ad8a3fb375da6cae83352bb74c7 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 coerce is not encouraged - lets just close this.
@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