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

All PRs merged and fixing all issues #65

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

covert-encryption
Copy link

@covert-encryption covert-encryption commented Nov 16, 2021

Merges #57 #62 #63 fixing conflicts and a few bugs introduced within. Additionally fixes the problem with empty password. Thus, all currently open PRs and issues are handled in this.

Fixes #56
Fixes #64

@covert-encryption
Copy link
Author

@dwolfhub Are you still maintaining this? We had to publish a fork because no PRs get merged here and those fixes were crucial for our primary project.

https://pypi.org/project/zxcvbn-covert/

@covert-encryption
Copy link
Author

@dwolfhub Getting a bit of mixed messages here, since you suddenly did merge one of the PRs but did not write anything here. If you'd prefer, we can take over the maintenance of this package. We do not plan to do any big changes, only bug fixes if any further issues are reported, aiming to closely follow the original Javascript implementation.

One of the PRs merged here bumped the version number to 5.0.0, so if you do release another version, I would suggest bumping your version number higher than that. The bump in major version is justified because support for old Python versions was dropped. If you intend to keep maintaining this, we will not push further versions to PyPi, to avoid further confusion with versions. Our package still installs under zxcvbn module name, so zxcvbn-covert conflicts with your zxcvbn-python.

@dwolfhub
Copy link
Owner

dwolfhub commented Dec 3, 2021

@covert-encryption thanks the submission.
i'd like to avoid one giant PR that includes lots of different changes, including merging multiple PRs into it..
i also have some questions about why you chose to use dictionaries instead of lists in some places and also changing date times to perf counters. it would be a lot easier, i think, if you submitted PRs for individual changes and helped me understand your reasoning.

@covert-encryption
Copy link
Author

@covert-encryption thanks the submission. i'd like to avoid one giant PR that includes lots of different changes, including merging multiple PRs into it..

This was a practical need for making a release, as the source PRs were in already in conflict and I wanted to have them merged before making any further fixes.

i also have some questions about why you chose to use dictionaries instead of lists in some places and also changing date times to perf counters.

I believe you mean the use of sets, as introduced by @septatrix in #57 and @fuhrysteve in #62. The former makes the ordering of the returned leet substitutions not matter (I did not check whether the test would otherwise fail). The latter uses a set to remove any duplicate entries of user input (although it has a side effect of also changing the order, possibly affecting scoring).

perf_counter is the appropriate way to measure runtime in Python, and should be preferred over calendar dates (already touched in #62). This has the unintended side effect that the runtime is now returned as a float rather than as timedelta, that needs to be fixed to maintain compatibility.

@covert-encryption
Copy link
Author

covert-encryption commented Dec 3, 2021

Now that is sorted out but some mypy issues remain. This is not something that I worked on but I can have a look at fixing them if you want (as a separate PR built on top of this).

foonoxous added 2 commits December 3, 2021 21:10
- Python 3.9.2 does not have `Match` in `typing.__all__`, causing a
crash when zxcvbn is imported. Later Python versions fix that bug.
- Asterisk imports are better avoided anyway.
@covert-encryption
Copy link
Author

The last commit is an important workaround to a Python 3.9.2 bug that prevents loading zxcvbn. While the problematic import introduced in #57 is technically correct and works on more recent Python versions (3.9.9 at least is OK), it is better to avoid asterisk imports entirely, and thus avoid this compatibility problem too.

This also increases the urgency of another release, so we are hoping that @dwolfhub would take action, as we really need all of these bugs fixed.

@covert-encryption
Copy link
Author

Development is moved to main branch to avoid pushing more commits on this PR (based on master of our fork). We just released zxcvbn-covert 0.5.1 with everything on this PR as well as some additional work to have typing checks pass cleanly, and a minor update of setup.py.

.github/workflows/build.yml Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@dwolfhub
Copy link
Owner

@covert-encryption I have some availability and would be happy to help get all your work released

@covert-encryption
Copy link
Author

@dwolfhub Cool. Do you wish for the typing fixes to be pushed on this PR as well, avoiding that test failure?

Also I (faintly) recall seeing some Python 3.8 feature being used in typing, so this might not work with 3.6 or 3.7 anymore. Need to have a look and get all the version numbers (CI, setup.py and readme) in sync.

@dwolfhub
Copy link
Owner

Yes let’s get the tests to pass and get it merged. I’d like to support 3.7-3.10.

@covert-encryption
Copy link
Author

Yes, https://docs.python.org/3/library/typing.html#typing.TypedDict which is heavily used on zxcvbn.types is new in Python 3.8. However, even Debian is already on 3.9, other distros already made 3.10 the default, MacOS ships at least 3.8 (cannot check the latest OS versions) and a very few people have older than 3.8 anywhere. Although this certainly could be a concern for some (corporate) users of the module.

* Fix or disable all typing errors.
* Sort imports.
@covert-encryption
Copy link
Author

That should do it for the tests. I did not sync version numbers yet.

@septatrix
Copy link

You could also add the official typing_extensions as a conditional dependency for older python versions

@covert-encryption
Copy link
Author

covert-encryption commented Dec 18, 2021

You could also add the official typing_extensions as a conditional dependency for older python versions

Would you wish to provide a patch for that? I don't even have anything older than 3.9 to test with :)

As far as other things go, it could still be supporting 3.6 (the bare minimum for typing and the ordered dict) or 3.7 but really needs actual testing to be sure (too many changes to review everything manually). TypedDict was just one thing I casually spotted.

@woodruffw
Copy link

Sorry to ping on an old PR, but is there any movement here?

I noticed that this was open while trying to determine if this library had 3.10 and newer in CI yet; it looks like 3.11 could also be added now.

Let me know if I can help at all; I'm happy to either help out with this branch or in any other way I can 🙂

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.

IndexError: list index out of range, on empty password Add type hints according to PEP 484
6 participants