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

Rework documentation of std.zip. #7255

Merged
merged 1 commit into from Nov 20, 2019
Merged

Rework documentation of std.zip. #7255

merged 1 commit into from Nov 20, 2019

Conversation

ghost
Copy link

@ghost ghost commented Oct 25, 2019

The format of the table in the main documentation is not yet perfect, but I could not find anything better. Especially I found no local link version, where the displayed value is different to the anchor value. I used REF1_ALTTEXT for this.

I left out the flags and internalAttributes of ArchiveMember, because I don't know exactly their meaning. As I have to investigate on this anyway, I'll fill this in, when I know better.

I would be happy if a native english speaker could correct any language mistakes I made.

std/zip.d Outdated

Standards:

The current implementation mostly conforms to $(LINK2 https://www.iso.org/standard/60101.html, ISO/IEC 21320-1:2015),
Copy link
Author

@ghost ghost Oct 25, 2019

Choose a reason for hiding this comment

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

I write "mostly", because there might be some nonconformation left due to UTF8 issues or with the flags. I'm not totally in this currently, but have to investigate soon for fixing some further bugs.

std/zip.d Outdated
unpack algorithm)
)

The current implementation makes use of the etc.c.zlib compression library.
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure, if this is still correct. The implementation uses std.zlib, which probably is based on etc.c.zlib. I'm not sure if this comment is of any help, but it was in the original documentation and so I decided to preserve it.

Copy link
Member

Choose a reason for hiding this comment

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

Neither etc.c.zlib nor std.zlib are a compression library, they merely wrap one. Probably it would be more correct if this was changed to either say that it uses the zlib library or the etc.c.zlib bindings / std.zlib wrapper for the zlib library.

@ghost ghost marked this pull request as ready for review October 25, 2019 14:50
@ghost ghost requested a review from CyberShadow as a code owner October 25, 2019 14:50
std/zip.d Outdated Show resolved Hide resolved
std/zip.d Outdated Show resolved Hide resolved
std/zip.d Outdated Show resolved Hide resolved
std/zip.d Outdated
unpack algorithm)
)

The current implementation makes use of the etc.c.zlib compression library.
Copy link
Member

Choose a reason for hiding this comment

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

Neither etc.c.zlib nor std.zlib are a compression library, they merely wrap one. Probably it would be more correct if this was changed to either say that it uses the zlib library or the etc.c.zlib bindings / std.zlib wrapper for the zlib library.

std/zip.d Show resolved Hide resolved
@ghost ghost requested a review from wilzbach as a code owner October 26, 2019 20:20
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7255"

std/zip.d Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Nov 20, 2019

Resolved merge conflict and changed Posix to POSIX.

@dlang-bot dlang-bot merged commit fc85b44 into dlang:master Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants