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

Add a bash-completion script (#22) #222

Closed
wants to merge 2 commits into from
Closed

Add a bash-completion script (#22) #222

wants to merge 2 commits into from

Conversation

jayrajput
Copy link
Contributor

I have made changes to the setup.py to include the auto completion script in the python package. I am not sure if I need to do that. Also if the version for the mozdownloand in the setup.py needs to be updated.

@whimboo
Copy link
Contributor

whimboo commented Jul 2, 2014

@jayrajput thank you for the updated patch! In general it looks fine but before I would like to get started with the review I want to know why exactly you dropped the optcomplete package. What is the overhead you are mentioning on issue #22? I checked some example code from that project, and it looks kinda easy to get implemented.

@jayrajput
Copy link
Contributor Author

The only overhead I saw was adding dependency on the optcomplete. And the functionality which we needed was done in four lines I added to the scrapper.py. Module optcomplete provide more fancier completion which we do not need (for e.g. file completion, director completion, known host completion). Also their bash completion code was complex.

Now as you are hinting towards usage of optocomplete, I can also think of advantages of optcomplet.
The module optcomplete is well documented and that provides more maintainability. So if adding dependency on a module is not an issue, I can rework on this by using optcomplete. I prefer to use optcomplete if adding dependency on a python module is not a problem..

@whimboo
Copy link
Contributor

whimboo commented Jul 3, 2014

Ok, so I checked the package on PyPI and it's not that large. So I don't think it would harm us adding this dependency. Lets get it added, so we do not re-invent the wheel, and can make use of the fancy other features you are referring to automatically. Thanks.

@whimboo
Copy link
Contributor

whimboo commented Sep 3, 2014

@jayrajput do you have an update for us? Have you had the time to do the changes?

@jayrajput
Copy link
Contributor Author

This went on a delay path. Let me work on this on weekend and send you an
update. I tried incorporating the optcomplete module, but that
failed. Thought I will debug it myself. I shall be able to work on this
over the weekend and send you an update.​

@whimboo
Copy link
Contributor

whimboo commented Sep 4, 2014

Thanks for the update Jay! If you have problems let me know.

@jayrajput
Copy link
Contributor Author

@whimboo Couple of issues which I need to discuss:

  1. During testing, I found that the tab completion is too slow. User has to wait for 2-3 seconds after hitting tab. The optcomplete module calls the mozdownload to find the completion and mozdownload takes 2-3 seconds to import all the needed modules. The requests module imported by scrapper.py is slowest to load. Note that this problem will also be present with my old changes. I did overlooked this problem while testing last time, or may be it much noticeable now because of the virtualized machine I have started using.
  2. I deleted the fork as I was having problems, not realizing that the PR is still open for the same. I hope this PR can be deleted. How can I delete this PR as the fork repo corresponding to this PR is already deleted. I created a new one.

Here are the logs, show delay in the import of requests. I am using print time.localtime to log timestamp

[tmp]$./venv/bin/python ./venv/bin/mozdownload
BEFORE: time.struct_time(tm_year=2014, tm_mon=9, tm_mday=6, tm_hour=13, tm_min=13, tm_sec=4, tm_wday=5, tm_yday=249, tm_isdst=0)
BEFORE SCRAPER IMPORT time.struct_time(tm_year=2014, tm_mon=9, tm_mday=6, tm_hour=13, tm_min=13, tm_sec=4, tm_wday=5, tm_yday=249, tm_isdst=0)
BEFORE requests time.struct_time(tm_year=2014, tm_mon=9, tm_mday=6, tm_hour=13, tm_min=13, tm_sec=5, tm_wday=5, tm_yday=249, tm_isdst=0)
AFTER requests time.struct_time(tm_year=2014, tm_mon=9, tm_mday=6, tm_hour=13, tm_min=13, tm_sec=7, tm_wday=5, tm_yday=249, tm_isdst=0)
AFTER SCRAPER IMPORT time.struct_time(tm_year=2014, tm_mon=9, tm_mday=6, tm_hour=13, tm_min=13, tm_sec=8, tm_wday=5, tm_yday=249, tm_isdst=0)
time.struct_time(tm_year=2014, tm_mon=9, tm_mday=6, tm_hour=13, tm_min=13, tm_sec=8, tm_wday=5, tm_yday=249, tm_isdst=0)

Module to handle downloads for different types of ftp.mozilla.org hosted applications.

mozdownload version: 1.11.1

Usage: mozdownload [options]

Specify --help for more information on options. Please see the README for examples.
AFTER: time.struct_time(tm_year=2014, tm_mon=9, tm_mday=6, tm_hour=13, tm_min=13, tm_sec=8, tm_wday=5, tm_yday=249, tm_isdst=0)

@Nebelhom
Copy link
Collaborator

Nebelhom commented Mar 6, 2015

@whimboo , @jayrajput

Hi folks! Spring cleaning ;) What is the status of this PR? Can it be closed or will we work on it in the future?

@jayrajput
Copy link
Contributor Author

@Nebelhom, @whimboo

The idea of using tab complete is not feasible with the optcomplete as the optcomplete module calls the mozdownload to find the completion and mozdownload takes 2-3 seconds to import all the needed modules. The slowest module to load is requests. Please see my previous comments for details.

Loading the modules conditionally is one possibility, but that will start getting tricky.
Any suggestion on how to implement this? I can start working on it.

@whimboo
Copy link
Contributor

whimboo commented Mar 11, 2015

Sorry for totally forgetting about this PR! And thanks to @Nebelhom to bring it back to my attention. I can now see the problems we would have here. That's really unfortunate. I wonder if we shouldn't import the requests module globally but only when we really need it. That would drastically help. I would suggest we investigate that in a new issue first. It can block issue #22.

Given that you have removed your fork you won't be able to push to this PR anymore. So lets close it.

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.

3 participants