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.

Sign up for GitHub

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

Jump to bottom

Add writeFile for Builder #408

Merged
merged 4 commits into from
Aug 5, 2021
Merged

Add writeFile for Builder #408

merged 4 commits into from
Aug 5, 2021

Conversation

3kyro
Copy link
Contributor

@3kyro 3kyro commented Aug 3, 2021 •
edited
Loading

#406
Some questions:

  • Should I replicate hPutBuilder tests? Seems like a repetition
  • Should I add readFile as well to better align the API? If yes I could add roundtrip tests similarly to the other modules

@3kyro 3kyro changed the title Add writeFile for Builder #406 Add writeFile for Builder Aug 3, 2021
Bodigrim
Bodigrim reviewed Aug 3, 2021
View reviewed changes
Data/ByteString/Builder.hs Outdated Show resolved Hide resolved
@Bodigrim
Copy link
Contributor

Bodigrim commented Aug 3, 2021

I'm not sure how useful is to readFile into Builder in general.

You can test that Lazy.readFile . Builder.writeFile matches toLazyByteString.

@3kyro
Copy link
Contributor Author

3kyro commented Aug 4, 2021

I'm not sure how useful is to readFile into Builder in general.

You can test that Lazy.readFile . Builder.writeFile matches toLazyByteString.

Thanks @Bodigrim, I've added the relevant test

@Bodigrim Bodigrim requested a review from sjakobi August 5, 2021 18:29
sjakobi
sjakobi approved these changes Aug 5, 2021
View reviewed changes
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.

Cheers!

@Bodigrim Bodigrim linked an issue Aug 5, 2021 that may be closed by this pull request
Add writeFileBuilder #406
Closed
@Bodigrim Bodigrim added this to the 0.11.2.0 milestone Aug 5, 2021
@Bodigrim Bodigrim merged commit 1010c9e into haskell:master Aug 5, 2021
@Bodigrim
Copy link
Contributor

Bodigrim commented Aug 5, 2021

Thanks!

Bodigrim added a commit to Bodigrim/bytestring that referenced this pull request Aug 7, 2021
@3kyro @Bodigrim
Add writeFile for Builder (haskell#408)
7ca9e8d 
* Add writeFile for `Builder` haskell#406

* Clarify imports in documentation

Co-authored-by: Bodigrim <[email protected]>

* Add test for `writeFile`

* Evaluate read bytestring before closing file

Co-authored-by: Bodigrim <[email protected]>
noughtmare pushed a commit to noughtmare/bytestring that referenced this pull request Dec 12, 2021
@3kyro @Bodigrim @noughtmare
Add writeFile for Builder (haskell#408)
08993fc 
* Add writeFile for `Builder` haskell#406

* Clarify imports in documentation

Co-authored-by: Bodigrim <[email protected]>

* Add test for `writeFile`

* Evaluate read bytestring before closing file

Co-authored-by: Bodigrim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjakobi sjakobi sjakobi approved these changes

@Bodigrim Bodigrim Bodigrim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.11.2.0
Development

Successfully merging this pull request may close these issues.

Add writeFileBuilder
3 participants
@3kyro @Bodigrim @sjakobi