python-validators / validators

Python Data Validation for Humans™.
MIT License
958 stars 152 forks source link

feat: add validator for eth addresses #383

Closed msamsami closed 2 months ago

msamsami commented 2 months ago
msamsami commented 2 months ago

Hi. Please give me feedback on whether this is something you would like to be added to the library. If so, I can make the additional changes for versioning, changelog, etc.

Please note that I plan to extend this by adding more validators for TRX, ADA, and other crypto addresses.

Thanks!

yozachar commented 2 months ago

Hi, I'll review, but is it possible to make the external dependency, optional?

msamsami commented 2 months ago

Hi, I'll review, but is it possible to make the external dependency, optional?

Sure. Something like a new group of optional dependencies named crypto, crypto-address, or crypto-addresses that includes all the dependencies related to validators of crypto addresses. What do you think? Please let me know your suggestions for the naming.

Finally, it can be used like pip install validators[crypto-addresses].

yozachar commented 2 months ago

That sound exactly like what I was thinking. pip install validators[crypto-eth-addresses]

msamsami commented 2 months ago

That sound exactly like what I was thinking. pip install validators[crypto-eth-addresses]

It's done. Now, how do you think we should update the pycqa.yaml workflow? I can explicitly include pip install .[crypto-eth-addresses] in "tooling" and "testing" jobs or create a new requirements.*.txt for it under package/ directory. Or is there a better way to do it? I'm trying to become close to your mindset here so that I can make more contributions with less confusion in the future. Thanks

yozachar commented 2 months ago

Change this:

https://github.com/python-validators/validators/blob/a7bcfda8902e0bcc42512c6630b0504ab9df65f0/package/roll.sh#L9-L10

to:

    # tooling
    pdm export --group tooling,crypto-eth-addresses -f requirements -o package/requirements.tooling.txt

Do the same in roll.ps1. Run either of one the scripts (bash or powershell, depending upon your OS), commit and push those changes to this PR.

Let's see if it works.

msamsami commented 2 months ago

Change this:

https://github.com/python-validators/validators/blob/a7bcfda8902e0bcc42512c6630b0504ab9df65f0/package/roll.sh#L9-L10

to:

    # tooling
    pdm export --group tooling,crypto-eth-addresses -f requirements -o package/requirements.tooling.txt

Do the same in roll.ps1. Run either of one the scripts (bash or powershell, depending upon your OS), commit and push those changes to this PR.

Let's see if it works.

It doesn't seem to be working. Isn't it because the optional dependencies are not installed here in the "testing" workflow?

https://github.com/python-validators/validators/blob/a7bcfda8902e0bcc42512c6630b0504ab9df65f0/.github/workflows/pycqa.yaml#L52-L53

yozachar commented 2 months ago

Ah, you are right, then let's add a redundant dependency group with:

$ pdm add -dG testing pytest

This should change pyproject.toml to include:

[tool.pdm.dev-dependencies]
+ testing = ["pytest>=8.2.2"]

Do not remove pytest from tooling, it still is required there. Then in roll.(sh|ps1) generate a new file like so:

# tooling
pdm export --group tooling -f requirements -o package/requirements.tooling.txt
+# testing
+pdm export --group testing,crypto-eth-addresses -f requirements -o package/requirements.testing.txt

Finally in pycqa.yaml, you can make the following change:

-run: pip install pytest
+run: pip install -r package/requirements.testing.txt

Run the scripts, commit, push and try again.

msamsami commented 2 months ago

@yozachar All tests are passed except for the tooling workflow. Seems like we need crypto-eth-addresses in tooling dependencies as well. What do you think?

# tooling
-pdm export --group tooling -f requirements -o package/requirements.tooling.txt
+pdm export --group tooling,crypto-eth-addresses -f requirements -o package/requirements.tooling.txt
# testing
pdm export --group testing,crypto-eth-addresses -f requirements -o package/requirements.testing.txt
yozachar commented 2 months ago

Ah yes, I missed that.

msamsami commented 2 months ago

It's all good now. Would you like to make a new release out of this PR? or after adding validators for more crypto addresses? If this counts as a release, I can update the SECURITY.md and src/validators/__init__.py files.

msamsami commented 2 months ago

This PR looks like a good candidate for 0.29.0. Please go ahead.

I updated the version and changelog. Let me know if this wraps it up or if more needs to be done.

yozachar commented 2 months ago

I'll push a few changes, just some arrangements.

yozachar commented 2 months ago

What do you think?

msamsami commented 2 months ago

What do you think?

Much better. Thanks!

yozachar commented 2 months ago

Thanks a lot for the PR!