Skip to content
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

Assorted documentation fixes #248

Merged
merged 8 commits into from
Jul 17, 2020
Merged

Assorted documentation fixes #248

merged 8 commits into from
Jul 17, 2020

Conversation

fumieval
Copy link
Contributor

@fumieval fumieval commented Jul 4, 2020

No description provided.

Data/ByteString/Unsafe.hs Outdated Show resolved Hide resolved
Data/ByteString/Lazy.hs Outdated Show resolved Hide resolved
Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :)

Data/ByteString/Internal.hs Outdated Show resolved Hide resolved
Data/ByteString/Internal.hs Outdated Show resolved Hide resolved
Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing I noticed.

Data/ByteString/Builder/Prim.hs Outdated Show resolved Hide resolved
-- > filter p =
-- > B.toLazyByteString .
-- > E.encodeLazyByteStringWithF (E.ifF p E.word8) E.emptyF)
-- >
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this because it has nothing to do with lists. A similar, and up-to-date example is already in the comment ofprimMapLazyByteStringBounded

Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thanks a lot, @fumieval! :)

@Bodigrim what do you think about the exposure of the internal Contravariant and Monoidal classes?

Data/ByteString/Builder/Prim.hs Outdated Show resolved Hide resolved
@Bodigrim
Copy link
Contributor

Bodigrim commented Jul 6, 2020

Honestly, I'm quite uncomfortable with internal modules exposed at all. It would be so much better to have a separate bytestring-internal package, but not much can be changed now. I would rather not make the situation worse by exposing more stuff, especially type classes, especially if the only benefit is fixing haddock links.

BTW what about adding a disclaimer to internal modules alike to http://hackage.haskell.org/package/containers-0.6.2.1/docs/Data-Map-Internal.html? This could simplify their maintenance after the next major bump.

@fumieval
Copy link
Contributor Author

fumieval commented Jul 7, 2020

OK, I removed the commit exposing Monoidal and Contravariant.

@sjakobi
Copy link
Member

sjakobi commented Jul 7, 2020

BTW what about adding a disclaimer to internal modules alike to http://hackage.haskell.org/package/containers-0.6.2.1/docs/Data-Map-Internal.html? This could simplify their maintenance after the next major bump.

I'm not very comfortable with just slipping this in in this PR. I think the stability guarantees and versioning policy for the Internal modules should be discussed on the issue tracker and possibly on the libraries list first.

My concern is that it feels like there was an implicit stability guarantee for these modules, and users may have based their code on such guarantees.

@Bodigrim
Copy link
Contributor

Bodigrim commented Jul 7, 2020

It would be nice to review which parts of internal modules proved to be useful and expose them through non-internal modules.

Copy link
Member

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :)

@Bodigrim
Copy link
Contributor

Bodigrim commented Jul 9, 2020

@hsyl20 could you please take a look as well?

@sjakobi
Copy link
Member

sjakobi commented Jul 17, 2020

@hsyl20 could you please take a look as well?

Since the changes are only in the documentation, and since there hasn't been any activity a week, I think 2 approvals are good enough.

If there are any problems in this PR, we can easily fix them later.

@hsyl20, maybe we should try to clarify which PRs should require your approval and which don't. The current situation is a bit vague.

@sjakobi sjakobi merged commit 93692cb into haskell:master Jul 17, 2020
@sjakobi
Copy link
Member

sjakobi commented Jul 17, 2020

Thanks again for all the fixes, @fumieval! :)

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants