UBC-MDS / software-review-2024

0 stars 0 forks source link

Passwordler - Group 15 #11

Open Kierst01 opened 9 months ago

Kierst01 commented 9 months ago

Submitting Author: Name (@Kierst01) All current maintainers: (@zywkloo, @rorywhite200, @mishelly-h) Package Name: Passwordler One-Line Description of Package: This package provides password management tools in Python. Repository Link: https://github.com/UBC-MDS/passwordler Version submitted: 2.0.0 Editor: @ttimbers
Reviewer 1: Hongyang Zhang
Reviewer 2: Mohammadsaleh Norouzi Reviewer 3: Shizhe Zhang Reviewer 4: Andy Zhang
Archive: TBD JOSS DOI: TBD Version accepted: TBD Date accepted (month/day/year): TBD


Code of Conduct & Commitment to Maintain Package

Description

This Python package is useful for users seeking an integrated solution for password management, offering a user-friendly experience. With key functionalities consolidated in one package, users can effortlessly generate strong passwords, evaluate their strength, and grasp encryption and decryption methods through our straightforward substitution cipher.

Scope

Domain Specific & Community Partnerships

- [ ] Geospatial
- [ ] Education
- [ ] Pangeo

Community Partnerships

If your package is associated with an existing community please check below:

[^1]: Please fill out a pre-submission inquiry before submitting a data visualization package.

Technical checks

For details about the pyOpenSci packaging requirements, see our packaging guide. Confirm each of the following by checking the box. This package:

Publication Options

JOSS Checks - [ ] The package has an **obvious research application** according to JOSS's definition in their [submission requirements][JossSubmissionRequirements]. Be aware that completing the pyOpenSci review process **does not** guarantee acceptance to JOSS. Be sure to read their submission requirements (linked above) if you are interested in submitting to JOSS. - [ ] The package is not a "minor utility" as defined by JOSS's [submission requirements][JossSubmissionRequirements]: "Minor ‘utility’ packages, including ‘thin’ API clients, are not acceptable." pyOpenSci welcomes these packages under "Data Retrieval", but JOSS has slightly different criteria. - [ ] The package contains a `paper.md` matching [JOSS's requirements][JossPaperRequirements] with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: *Note: JOSS accepts our review as theirs. You will NOT need to go through another full review. JOSS will only review your paper.md file. Be sure to link to this pyOpenSci issue when a JOSS issue is opened for your package. Also be sure to tell the JOSS editor that this is a pyOpenSci reviewed package once you reach this step.*

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

This option will allow reviewers to open smaller issues that can then be linked to PR's rather than submitting a more dense text based review. It will also allow you to demonstrate addressing the issue via PR links.

Confirm each of the following by checking the box.

Please fill out our survey

P.S. Have feedback/comments about our review process? Leave a comment here

Editor and Review Templates

The editor template can be found here.

The review template can be found here.

andyzhangstat commented 9 months ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 2 hours


Review Comments

The package is a very interesting python package, I really enjoy reading the documents and play with the functions. The content in current package is quite comprehensive and professional!

However, there are a few places can be improved.

  1. There is no comparison with existed python package which has similar functionality. Add a comparison with existing Python packages offering similar functionality to enhance completeness.
  2. There is no reference or explanation for the underlying logic of the password generation and encryption. Include a reference or brief explanation of the underlying logic of password generation and encryption in the tutorial.
  3. Some badges is missing in the readme part. Address missing badges in the readme for a future package version.
  4. Last but most importantly, the function in _internals.py is little bit wordy. Optimize the function in _internals.py by utilizing built-in functions like string.ascii to replace the original dictionary.
  5. It's better to add more explanation in the readme file. Enhance the readme by adding more function arguments along with brief explanations for each.
alexzhang0825 commented 9 months ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 1.5


Review Comments

  1. I think this was a really fun, well-executed and well-documented function package. It stated its purpose very clearly and implemented the functions very succinctly. The installation instructions and documentations were very easy to follow. The story was also very interesting and well-integrated in the example usage.

  2. In the src folder, as well as the tests folder, there are two empty files passwordler and test_passwordler. I believe these files were automatically generated during the early stage of development, and believe they can be removed.

  3. Recall that in 521 we were actually advised that a password consists of gibberish (mixed with numbers, symbols, different cases) is actually a lot easier to be guessed by computer and a lot harder to be remembered by people, whereas as simple password such as "correct horse battery staple" is completely opposite, despite intuitivey we would think the latter is a lot easier to be cracked by computers. The strength detection algorithm, like many others, uses the presence of symbols, cases, number, and length as the metrics, whereas this may actually lead to a weaker password. This problem is even worse manifested in generate_password(), where most of the passwords it generate are just random gibbrishes of numbers and symbols. I wanted to point this out, but knowing that to make it better it will likely require a much more complicated algorithm, which is beyond the scope of this course, so I think for the purpose of it you guys have done more than enough.

  4. In the README.md file, the functions could have been enclosed with the "`" symbol to make it clearer.

  5. Github Issues was very rarely used during the development. Given how well the package is done I suspect the communications were done mostly outside of it, but to get used to the Github workflow I would suggest nurturing the habbit of talking over Github Issues (Maybe in the final milestone use it more often). Turns out it was used very avidly, I forgot to check that they are closed.

MoNorouzi23 commented 9 months ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also submitting to JOSS

Not Applicable

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 1.5 h.

Review Comments

I explored your passwordler package, and I must say that it is inspiring. The organization of the package is impressive, and I've identified some aspects that I believe could be applied to enhance our group project. The concept of generating and managing passwords, with a focus on encryption and decryption, is an outstanding feature and contributes to the uniqueness of the package.

I do have a few suggestions that I believe could further improve the package:

I appreciate the effort and creativity of your team in developing such a useful package. Well done!

zhang-shizhe commented 9 months ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Readme file requirements The package meets the readme requirements below:

The README should include, from top to bottom:

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole. Package structure should follow general community best-practices. In general please consider whether:

Functionality

For packages also submitting to JOSS

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

Final approval (post-review)

Estimated hours spent reviewing: 1.5 h

Review Comments

  1. First, great job on the project! The documentation, especially the tutorial, is very clear and user-friendly. The examples are also really helpful.

  2. I noticed a couple of empty scripts in the src and test folders. If these aren't for future updates, it might be best to remove them to keep things tidy.

  3. The decrypt_password and encrypt_password functions both set up the random seed and mix up the original list on their own. This could cause trouble if you decide to change how the list is mixed or what's in it. It would be simpler and safer to make a separate function that sets up the list once and then use that list in both encryption and decryption.

The project is already well-made and well-explained. The next few points are just ideas you might consider for future updates, maybe after this phase of the project:

  1. The password_strength function uses a fixed list of common passwords. This can be hard to keep up-to-date and might not be secure since common passwords change a lot. Maybe use an API that keeps this list updated or finds a better way to check if a password is strong.

  2. It might be good to switch to a stronger encryption method, like AES (Advanced Encryption Standard), and make sure the randomness you use is really secure.

  3. Adding a logo for your package, maybe created with AI, could make it stand out more.