karolyi
/
py3-validate-email
2
1
Fork 3
Code Issues 1 Pull Requests Projects Releases Wiki Activity

Labels Milestones

Clean up initial blacklist download on install #14

Merged
reinhard-mueller merged 1 commits from clean-setup into master 3 years ago
Conversation 10 Commits 1 Files Changed 7
reinhard-mueller commented 3 years ago (Migrated from github.com)

I guess you are already tired of the whole issue around setuptools and the initial download of the blacklist :-/. However, if some day you have some fresh energy you might want to have a look at this patch, it might be the clean and straightforward solution that you may have been looking for.

I have made two essential changes to the process of initially downloading the blacklist at install time, allowing the removal of a number of hacks and workarounds:

  1. Download the files after populating the build directory instead of before. Thus, the files don't have to be included in MANIFEST.in, and following from that, setup.py sdist needn't be tricked into not including them in the source distribution.

  2. Only import the updater module instead of the full validate_email package, so setup.py install and friends don't depend on all the requirements of the library.

As usual, any kind of feedback is always appreciated!

I guess you are already tired of the whole issue around setuptools and the initial download of the blacklist :-/. However, if some day you have some fresh energy you might want to have a look at this patch, it might be the clean and straightforward solution that you may have been looking for. I have made two essential changes to the process of initially downloading the blacklist at install time, allowing the removal of a number of hacks and workarounds: 1. Download the files *after* populating the build directory instead of *before*. Thus, the files don't have to be included in MANIFEST.in, and following from that, `setup.py sdist` needn't be tricked into not including them in the source distribution. 2. Only import the updater module instead of the full validate_email package, so `setup.py install` and friends don't depend on all the requirements of the library. As usual, any kind of feedback is always appreciated!
karolyi commented 3 years ago (Migrated from github.com)

Feeling sedulous?

You might want to check travis, because stuff is failing ...

Feeling sedulous? You might want to check travis, because stuff is failing ...
karolyi commented 3 years ago (Migrated from github.com)

... and when you arrive to the same conclusions I did, you'll realize why I spent a day with this stuff, and why it is the way it is :)

Like I said: python packaging (distutils/setuptools) is a mess. Nevertheless, feel free to play around with it and see if you can find a solution to putting the data directory in at install time, that is better than mine.

... and when you arrive to the same conclusions I did, you'll realize why I spent a day with this stuff, and why it is the way it is :) Like I said: python packaging (distutils/setuptools) is a mess. Nevertheless, feel free to play around with it and see if you can find a solution to putting the data directory in at install time, that is better than mine.
karolyi (Migrated from github.com) reviewed 3 years ago
.travis.yml
@ -12,12 +12,11 @@ python:
install:
karolyi (Migrated from github.com) commented 3 years ago

Nope!

The point here is to install the package as if it were installed from pypi. setup.py develop is not the way of doing it!

Nope! The point here is to install the package as if it were installed from pypi. `setup.py develop` is not the way of doing it!
reinhard-mueller (Migrated from github.com) reviewed 3 years ago
.travis.yml
@ -12,12 +12,11 @@ python:
install:
reinhard-mueller (Migrated from github.com) commented 3 years ago

Okay, understood!

Okay, understood!
karolyi (Migrated from github.com) reviewed 3 years ago
validate_email/updater.py
@ -106,4 +90,4 @@
def _process(self, force: bool = False):
'Start optionally updating the blacklist.txt file, while locked.'
if not force and not self._is_old:
karolyi (Migrated from github.com) commented 3 years ago

This here serves a purpose, an edge case.

If the preinstalled file path doesn't exist (for example at install time), it should return True so that the updater will start an update. Besides, your updated version does return None, which is not a strict bool just boolish.

This here serves a purpose, an edge case. If the preinstalled file path doesn't exist (for example at install time), it should return `True` so that the updater will start an update. Besides, your updated version does return `None`, which is not a strict `bool` just boolish.
reinhard-mueller (Migrated from github.com) reviewed 3 years ago
validate_email/updater.py
@ -106,4 +90,4 @@
def _process(self, force: bool = False):
'Start optionally updating the blacklist.txt file, while locked.'
if not force and not self._is_old:
reinhard-mueller (Migrated from github.com) commented 3 years ago

Yes, right, there's a "return True" at the end of my version missing. In practice, this should never happen (this code is not run at install time any more), but I absolutely agree that it's better to have it there.

Yes, right, there's a "return True" at the end of my version missing. In practice, this should never happen (this code is not run at install time any more), but I absolutely agree that it's better to have it there.
reinhard-mueller commented 3 years ago (Migrated from github.com)

Sorry about the initial mess in the travis checks, I'm fairly new to travis. I think it should be ok now.

Sorry about the initial mess in the travis checks, I'm fairly new to travis. I think it should be ok now.
karolyi commented 3 years ago (Migrated from github.com)

Could you merge master into this?

Could you merge master into this?
reinhard-mueller commented 3 years ago (Migrated from github.com)

Could you merge master into this?

Sure, done!

> Could you merge master into this? Sure, done!
karolyi commented 3 years ago (Migrated from github.com)

I will have to look into this somewhat deeper because I think your changes are taking out details I intentionally built in. I'll check back on you.

I will have to look into this somewhat deeper because I think your changes are taking out details I intentionally built in. I'll check back on you.
reinhard-mueller commented 3 years ago (Migrated from github.com)

Sure, take your time, and don't hesitate to send me questions, comments or criticism!

Sure, take your time, and don't hesitate to send me questions, comments or criticism!
karolyi (Migrated from github.com) requested changes 3 years ago
karolyi (Migrated from github.com) left a comment

Please correct the mentioned things, other than that, seems good to me.

Seems like a fresh set of eyes can bring improvement on this. It still boggles my mind how complicated the setup process is.

Please correct the mentioned things, other than that, seems good to me. Seems like a fresh set of eyes can bring improvement on this. It still boggles my mind how complicated the setup process is.
setup.py
karolyi (Migrated from github.com) commented 3 years ago 
def run_initial_updater(path: Path):
```suggestion def run_initial_updater(path: Path): ```
setup.py
@ -81,0 +46,4 @@
store it into the build directory. A subsequent 'install' step will
copy the full contents of the build directory to the install
target, thus including the blacklist.
"""
karolyi (Migrated from github.com) commented 3 years ago

Please store and restore the original value of dont_write_bytecode, in order to not interfere with other installers.

Please store and restore the original value of `dont_write_bytecode`, in order to not interfere with other installers.
setup.py
karolyi (Migrated from github.com) commented 3 years ago

The dependencies of this package (idna, dnspython, filelock) are missing upon installation. Try and install the sdist package in a new venv, and you'll see them being not installed. You removed the dependencies from here.

The dependencies of this package (idna, dnspython, filelock) are missing upon installation. Try and install the sdist package in a new venv, and you'll see them being not installed. You removed the dependencies from here.
validate_email/updater.py
karolyi (Migrated from github.com) commented 3 years ago

I'd call this _install, signifying it's only intended for internal use.

I'd call this `_install`, signifying it's only intended for internal use.
karolyi (Migrated from github.com) reviewed 3 years ago
.travis.yml
@ -18,3 +16,4 @@
- python -Wd -m pip install -v dist/py3-validate-email-*.tar.gz
# command to run tests
script:
karolyi (Migrated from github.com) commented 3 years ago 
  - python -m unittest discover -v -s tests

-I isolates the user environment (~/.local/lib... under linux), not the virtual env you intended, I think. Even if there's no venv in the case of travis tests, it makes no difference in isolating python here. The point of the InstallTest.test_datadir_is_in_place is to not use the locally deployed validate_email directory, hence the sys.path.remove("") in the test.

```suggestion - python -m unittest discover -v -s tests ``` `-I` isolates the user environment (`~/.local/lib...` under linux), not the virtual env you intended, I think. Even if there's no venv in the case of travis tests, it makes no difference in isolating python here. The point of the `InstallTest.test_datadir_is_in_place` is to not use the locally deployed `validate_email` directory, hence the `sys.path.remove("")` in the test.
reinhard-mueller (Migrated from github.com) reviewed 3 years ago
validate_email/updater.py
reinhard-mueller (Migrated from github.com) commented 3 years ago

Okay.

Okay.
reinhard-mueller (Migrated from github.com) reviewed 3 years ago
setup.py
reinhard-mueller (Migrated from github.com) commented 3 years ago

Do you have a tool to check the completeness of type annotations, or are you just so attentive? I really try to pay attention but I seem to forget this at least in one place for each of my commits :-/

Do you have a tool to check the completeness of type annotations, or are you just so attentive? I really try to pay attention but I seem to forget this at least in one place for each of my commits :-/
reinhard-mueller (Migrated from github.com) reviewed 3 years ago
setup.py
@ -81,0 +46,4 @@
store it into the build directory. A subsequent 'install' step will
copy the full contents of the build directory to the install
target, thus including the blacklist.
"""
reinhard-mueller (Migrated from github.com) commented 3 years ago

Okay.

Okay.
reinhard-mueller (Migrated from github.com) reviewed 3 years ago
setup.py
reinhard-mueller (Migrated from github.com) commented 3 years ago

Sorry, I was tricked by the fact that in .travis.yml, the dependencies were explicitly installed. This is because previously, the install process itself depended on them, which is no longer the case, so I'll remove that one line.

Does this leave any reason to keep requirements.txt at all, or would it be preferrable to have the dependency list directly in setup.py? The latter seems to be what most libraries do, AFAICT.

Sorry, I was tricked by the fact that in .travis.yml, the dependencies were explicitly installed. This is because previously, the install process itself depended on them, which is no longer the case, so I'll remove that one line. Does this leave any reason to keep requirements.txt at all, or would it be preferrable to have the dependency list directly in setup.py? The latter seems to be what most libraries do, AFAICT.
karolyi (Migrated from github.com) reviewed 3 years ago
setup.py
karolyi (Migrated from github.com) commented 3 years ago

One could use mypy for that, I don't because I couldn't set it up properly with Django.

I use python-language-server with sublime, and that will show completions for function input parameters only if the type is specified there. So basically it's grown to be a habit for me.

One could use mypy for that, I don't because I couldn't set it up properly with Django. I use python-language-server with sublime, and that will show completions for function input parameters only if the type is specified there. So basically it's grown to be a habit for me.
karolyi (Migrated from github.com) reviewed 3 years ago
setup.py
karolyi (Migrated from github.com) commented 3 years ago

I don't remember why I put it into requirements.txt, but it's fine to specify them in setup.py.

I don't remember why I put it into requirements.txt, but it's fine to specify them in setup.py.
reinhard-mueller commented 3 years ago (Migrated from github.com)

All the changes should be done now.

@karolyi I recommend that before doing a release you make sure you delete the directory py3-validate-email.egg-info because that contains some information which now is not correct any more.

Regarding the requirements, I moved them to setup.py as they were. I'm not sure whether there is a specific reason to have the exact versions there (with ==), I guess users of the library would be happier if it was >=.

Please let me know whether I should squash into a single commit again before you merge.

All the changes should be done now. @karolyi I recommend that before doing a release you make sure you delete the directory py3-validate-email.egg-info because that contains some information which now is not correct any more. Regarding the requirements, I moved them to setup.py as they were. I'm not sure whether there is a specific reason to have the exact versions there (with ==), I guess users of the library would be happier if it was >=. Please let me know whether I should squash into a single commit again before you merge.
karolyi (Migrated from github.com) reviewed 3 years ago
setup.py
@ -87,3 +54,4 @@
run_initial_updater(Path(self.build_lib))
setup(
karolyi (Migrated from github.com) commented 3 years ago 
    install_requires=['dnspython~=1.16', 'idna~=2.8', 'filelock~=3.0'],
```suggestion install_requires=['dnspython~=1.16', 'idna~=2.8', 'filelock~=3.0'], ```
karolyi (Migrated from github.com) reviewed 3 years ago
.travis.yml
karolyi (Migrated from github.com) commented 3 years ago 
  - python -Wd setup.py sdist -v
  - python -Wd -m pip install -v dist/py3-validate-email-*.tar.gz
```suggestion - python -Wd setup.py sdist -v - python -Wd -m pip install -v dist/py3-validate-email-*.tar.gz ```
karolyi commented 3 years ago (Migrated from github.com)

Hey,

I made some changes regarding version usage and travis output, please squash and we're good to go.

Hey, I made some changes regarding version usage and travis output, please squash and we're good to go.
reinhard-mueller commented 3 years ago (Migrated from github.com)

Squash done! Thank you for your additional improvements!

Squash done! Thank you for your additional improvements!
karolyi (Migrated from github.com) approved these changes 3 years ago

Reviewers

karolyi
The pull request has been merged as 79e59ee4b7.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b clean-setup master
git pull origin clean-setup

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff clean-setup
git push origin master
Sign in to join this conversation.
Reviewers
Request review
No reviewers
karolyi
Labels
Apply labels
Clear labels   
bug

Something isn't working   
duplicate

This issue or pull request already exists   
enhancement

New feature or request   
good first issue

Good for newcomers   
help wanted

Extra attention is needed   
invalid

This doesn't seem right   
question

Further information is requested   
stale
   
wontfix

This will not be worked on
No Label
bug
duplicate
enhancement
good first issue
help wanted
invalid
question
stale
wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Set Project
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: karolyi/py3-validate-email#14
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'clean-setup'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes