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

Pre-review-review #1

Open
jfy133 opened this issue Apr 4, 2024 · 3 comments
Open

Pre-review-review #1

jfy133 opened this issue Apr 4, 2024 · 3 comments

Comments

@jfy133
Copy link

jfy133 commented Apr 4, 2024

This review comes from a user point of view:

Overall

Very nice! Concept is simple, and works well in a fun manner :).

Feedback and suggested changes

  • Keep all function names in the same format: snake case or camel case, but not a mix (basically: keepitsafe() -> keep_it_safe())
  • keepitsafe() I don't really understand the differences between the word/phrqase/sentence/random -> word gives multiuple random words when I expect a single word?
  • keepitsafe() -> word mode, I think many password validators don't allow spaces as a valid character, maybe make it optional (off by default)
    -keepitsafe() -> help message doesn't descrie the different methods, so why is e.g. sentence and random being reduced? Longer is generally better and more memorable
  • is_it_secret() needs a 'human readable' outcome message, similar to is_it_safe() (the 'all right cousin frodo' is only LOTR-nerd understandable I think)
  • Any concerns about copyright, or is it out of it now (assuming yes as you have the raw text...?)

But otherwise love it :D

@bbartholdy
Copy link
Owner

bbartholdy commented May 14, 2024

To be honest, keepitsafe() was just because is looked better with keepitsecret::keepitsafe(). But isitsecret() and isitsafe() did not... I'll probably just add keep_it_safe() as an alias.

@jfy133
Copy link
Author

jfy133 commented May 23, 2024

Also works :D consistency one-way or another.

FWIW I personally find isitsecret() and isitsafe() fine to read 👍

@bbartholdy
Copy link
Owner

I decided to stick to R convention and separate words with an underscore.

Word method now has a hyphen by default, and the secret function has a more user-friendly message. There will also be a vignette explaining the different methods.

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

No branches or pull requests

2 participants