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

Doc patch: link to ShortByteString from ByteString module #609

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

ulidtko
Copy link
Contributor

@ulidtko ulidtko commented Aug 13, 2023

Greetings all

Per the closed discussion in #193 — code-change won't be happening, but documentation may get improved.

Here I'm submitting a paragraph near the top of Data.ByteString module reiterating the points made in Data.ByteString.Short section on memory fragmentation. The idea is to simply advertise ShortByteString from the package entry-point.

Having been caught in this caveat myself — I can definitely reflect that the mention of ShortByteString from package header (cabal description), was hard to notice.

@Bodigrim please review? Wording nitpicks, feedback, etc — totally welcome. Would appreciate an approve to merge, thanks in advance.

@Bodigrim Bodigrim requested a review from clyring August 13, 2023 15:56
@clyring
Copy link
Member

clyring commented Aug 16, 2023

I like the general idea. I'll try to leave some wordsmithery comments in the next day or two.

Data/ByteString.hs Outdated Show resolved Hide resolved
Data/ByteString.hs Outdated Show resolved Hide resolved
Data/ByteString.hs Outdated Show resolved Hide resolved
@ulidtko ulidtko force-pushed the docs/mention-shortbytestring branch from 85f1e08 to b2b36b5 Compare August 18, 2023 09:23
@ulidtko
Copy link
Contributor Author

ulidtko commented Aug 18, 2023

Hi @clyring, thanks for review. Suggestions taken, patch amended with rewords.

Copy link
Member

@clyring clyring left a comment

Choose a reason for hiding this comment

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

Sorry for the high latency on this round of review.

Data/ByteString.hs Outdated Show resolved Hide resolved
Data/ByteString.hs Outdated Show resolved Hide resolved
@ulidtko
Copy link
Contributor Author

ulidtko commented Aug 27, 2023

No worries at all @clyring, thanks for getting back on this, appreciate the thoroughness.

I'd responded in comments trying to clarify, and remain open for further suggestions.

It's overall an intricate topic, but I think the docs here shouldn't elaborate all related technicalities; what remains is necessarily vague hinting at ideas, search keywords and such. I'll readily admit it's a fine balance, and won't dispute your perspective on which way to tilt it. So, by all means, smash the word anvil 😃 as you see fit.

@ulidtko ulidtko force-pushed the docs/mention-shortbytestring branch from b2b36b5 to 8a72621 Compare September 5, 2023 09:31
@ulidtko
Copy link
Contributor Author

ulidtko commented Sep 5, 2023

Took another stab at it, amended with minor tweaks + rebased onto latest master.

@ulidtko ulidtko requested a review from clyring September 5, 2023 09:37
@Bodigrim Bodigrim merged commit 6e6b115 into haskell:master Sep 8, 2023
24 checks passed
@ulidtko ulidtko deleted the docs/mention-shortbytestring branch September 11, 2023 09:04
@clyring clyring added this to the 0.12.1.0 milestone Jan 30, 2024
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.

3 participants