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

sha1: Updated documentation #10374

Merged
merged 6 commits into from
Jan 19, 2019

Conversation

ThomasTJdev
Copy link
Contributor

Updated documentation for sha1 module:

  • Intro text
  • Code samples
  • See also links
  • Runnable examples

refs #10330
CC @narimiran

lib/std/sha1.nim Outdated Show resolved Hide resolved
lib/std/sha1.nim Outdated Show resolved Hide resolved
@narimiran narimiran self-assigned this Jan 19, 2019
Copy link
Member

@narimiran narimiran left a comment

Choose a reason for hiding this comment

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

Some minor changes to do and we can merge this.

lib/std/sha1.nim Outdated
result = ""
for v in Sha1Digest(self):
result.add(toHex(int(v), 2))

proc parseSecureHash*(hash: string): SecureHash =
## Converts a string to a SecureHash
Copy link
Member

@narimiran narimiran Jan 19, 2019

Choose a reason for hiding this comment

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

I never used this module, so at first I had hard time understanding the difference between this proc and secureHash — both seem to convert string to SecureHash.

Maybe it would be better/clearer to have here "Converts a string hash to a SecureHash"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I do agree - much clearer. I have also used backticks on the other instances of SecureHash.

dac416a

lib/std/sha1.nim Outdated
@@ -170,23 +195,46 @@ proc finalize(ctx: var Sha1State): Sha1Digest =
# Public API

proc secureHash*(str: string): SecureHash =
## Generates a SecureHash from a string and returns it.
Copy link
Member

Choose a reason for hiding this comment

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

I would have either "Generates and returns a SecureHash from a string str", or remove "returns" completely: "Generates a SecureHash from a string str"


I'll repeat a comment from uri:

General remark: I would love to see "See also:" section which will give links to similar procs. Some examples: strutils.indent, tables.getOrDefault, etc.

Here, specifically, a link to secureHashFile and parseSecureHash would be useful. (And vice-versa)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Text updated in dac416a

Links updated in 331550f

I think I'm coming to terms with the internal linking 😄 . Give me a second to update PR #10372 for algorithm, so I can add some more links. I'm glad for the review comments and to be able to contribute in some way, but please let me know, if this review process takes too much of your time instead of doing it yourself.

Copy link
Member

Choose a reason for hiding this comment

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

but please let me know, if this review process takes too much of your time instead of doing it yourself.

I have plenty of documentation I need to work on my own, so doing some reviews is a welcome change :)

So it is better for me this way (and more rewarding to you :)), and I'm sure as the time goes by this will take both us less time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you - and I'm also convinced of that ;)

lib/std/sha1.nim Outdated Show resolved Hide resolved
@narimiran narimiran merged commit 4a04470 into nim-lang:devel Jan 19, 2019
ThomasTJdev added a commit to ThomasTJdev/Nim that referenced this pull request Jan 27, 2019
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