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

feat: add validator for eth addresses #383

Merged
merged 10 commits into from
Jul 1, 2024

Conversation

msamsami
Copy link
Contributor

  • A new validator is added for the Ethereum address
  • Crypto address validators combined into a new directory: src/validators/crypto_addresses

@msamsami
Copy link
Contributor Author

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!

@msamsami msamsami changed the title Add validator for ETH addresses feat: add validator for ETH addresses Jun 25, 2024
@msamsami msamsami changed the title feat: add validator for ETH addresses feat: add validator for eth addresses Jun 25, 2024
@yozachar
Copy link
Collaborator

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

@msamsami
Copy link
Contributor Author

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
Copy link
Collaborator

yozachar commented Jun 28, 2024

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

@msamsami
Copy link
Contributor Author

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
Copy link
Collaborator

yozachar commented Jun 29, 2024

Change this:

# tooling
pdm export --group tooling -f requirements -o package/requirements.tooling.txt

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
Copy link
Contributor Author

Change this:

# tooling
pdm export --group tooling -f requirements -o package/requirements.tooling.txt

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?

- name: Install 'testing' dependencies
run: pip install pytest

@yozachar
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Collaborator

yozachar commented Jun 30, 2024

Ah yes, I missed that.

@msamsami
Copy link
Contributor Author

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.

Copy link
Collaborator

@yozachar yozachar left a comment

Choose a reason for hiding this comment

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

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

@msamsami
Copy link
Contributor Author

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
Copy link
Collaborator

yozachar commented Jul 1, 2024

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

@yozachar
Copy link
Collaborator

yozachar commented Jul 1, 2024

What do you think?

@yozachar yozachar self-assigned this Jul 1, 2024
@yozachar yozachar added enhancement Issue/PR: A new feature dependencies PR: Update/Remove a (dev/optional) dependencies labels Jul 1, 2024
- make `eth_hash` truly optional
- update dev dependencies
- improve changelog
- fix packaging and CI
@msamsami
Copy link
Contributor Author

msamsami commented Jul 1, 2024

What do you think?

Much better. Thanks!

@yozachar yozachar merged commit 4973c49 into python-validators:master Jul 1, 2024
17 checks passed
@yozachar
Copy link
Collaborator

yozachar commented Jul 1, 2024

Thanks a lot for the PR!

bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Jul 2, 2024
https://build.opensuse.org/request/show/1184350
by user mia + anag+factory
- Update to 0.29.0
Breaking:
  * move btc_address to crypto_addresses
    gh#python-validators/validators#383
Features:
  * add validator for eth addresses
    (depends on eth-hash, which is not packaged yet)
    gh#python-validators/validators#383
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies PR: Update/Remove a (dev/optional) dependencies enhancement Issue/PR: A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants