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

Clean up initial blacklist download on install #14

Merged
reinhard-mueller merged 1 commits from clean-setup into master 2020-04-14 12:24:31 +02:00
Conversation 10 Commits 1 Files Changed 7 +87 -154
reinhard-mueller commented 2020-04-13 19:19:37 +02:00 (Migrated from github.com)
Copy Link

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 2020-04-13 19:23:02 +02:00 (Migrated from github.com)
Copy Link

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 2020-04-13 19:25:30 +02:00 (Migrated from github.com)
Copy Link

... 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 2020-04-13 21:30:41 +02:00
.travis.yml
@ -12,12 +12,11 @@ python:
install:
karolyi (Migrated from github.com) commented 2020-04-13 21:30:41 +02:00
Copy Link

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 2020-04-13 21:46:47 +02:00
.travis.yml
@ -12,12 +12,11 @@ python:
install:
reinhard-mueller (Migrated from github.com) commented 2020-04-13 21:46:47 +02:00
Copy Link

Okay, understood!

Okay, understood!
karolyi (Migrated from github.com) reviewed 2020-04-13 22:08:28 +02:00
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 2020-04-13 22:08:28 +02:00
Copy Link

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 2020-04-13 22:18:38 +02:00
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 2020-04-13 22:18:38 +02:00
Copy Link

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 2020-04-13 22:20:46 +02:00 (Migrated from github.com)
Copy Link

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 2020-04-13 22:22:04 +02:00 (Migrated from github.com)
Copy Link

Could you merge master into this?

Could you merge master into this?
reinhard-mueller commented 2020-04-13 22:28:09 +02:00 (Migrated from github.com)
Copy Link

Could you merge master into this?

Sure, done!

> Could you merge master into this? Sure, done!
karolyi commented 2020-04-13 22:33:23 +02:00 (Migrated from github.com)
Copy Link

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 2020-04-13 22:35:45 +02:00 (Migrated from github.com)
Copy Link

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 2020-04-13 23:17:59 +02:00
karolyi (Migrated from github.com) left a comment
Copy Link

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 2020-04-13 23:03:51 +02:00
Copy Link
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 2020-04-13 23:04:59 +02:00
Copy Link

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 2020-04-13 23:12:56 +02:00
Copy Link

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 2020-04-13 22:59:48 +02:00
Copy Link

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 2020-04-13 23:44:33 +02:00
.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 2020-04-13 23:44:33 +02:00
Copy Link
  - 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 2020-04-14 08:45:14 +02:00
validate_email/updater.py
reinhard-mueller (Migrated from github.com) commented 2020-04-14 08:45:14 +02:00
Copy Link

Okay.

Okay.
reinhard-mueller (Migrated from github.com) reviewed 2020-04-14 08:47:09 +02:00
setup.py
reinhard-mueller (Migrated from github.com) commented 2020-04-14 08:47:09 +02:00
Copy Link

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 2020-04-14 08:47:26 +02:00
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 2020-04-14 08:47:26 +02:00
Copy Link

Okay.

Okay.
reinhard-mueller (Migrated from github.com) reviewed 2020-04-14 08:56:56 +02:00
setup.py
reinhard-mueller (Migrated from github.com) commented 2020-04-14 08:56:56 +02:00
Copy Link

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 2020-04-14 10:06:02 +02:00
setup.py
karolyi (Migrated from github.com) commented 2020-04-14 10:06:02 +02:00
Copy Link

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 2020-04-14 10:07:06 +02:00
setup.py
karolyi (Migrated from github.com) commented 2020-04-14 10:07:06 +02:00
Copy Link

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 2020-04-14 11:26:42 +02:00 (Migrated from github.com)
Copy Link

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 2020-04-14 11:48:31 +02:00
setup.py
@ -87,3 +54,4 @@
run_initial_updater(Path(self.build_lib))
setup(
karolyi (Migrated from github.com) commented 2020-04-14 11:48:30 +02:00
Copy Link
    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 2020-04-14 12:02:27 +02:00
.travis.yml
karolyi (Migrated from github.com) commented 2020-04-14 12:02:27 +02:00
Copy Link
  - 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 2020-04-14 12:11:40 +02:00 (Migrated from github.com)
Copy Link

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 2020-04-14 12:20:01 +02:00 (Migrated from github.com)
Copy Link

Squash done! Thank you for your additional improvements!

Squash done! Thank you for your additional improvements!
karolyi (Migrated from github.com) approved these changes 2020-04-14 12:23:48 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
karolyi
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
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
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
No description provided.
Delete Branch "clean-setup"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?