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

Refactor distributor modules to classes #262

Merged
merged 29 commits into from
Jun 7, 2018
Merged

Conversation

mmmaisel
Copy link
Contributor

@mmmaisel mmmaisel commented Jun 4, 2018

Refactoring as discussed in issue #242. All regressions should be fixed by now.
Random samples from the tests directory (test2.xml, test3.xml, Decoder.xml) work without errors.

This closes issues #242, #174, #215

mmmaisel and others added 24 commits June 4, 2018 17:08
Goals are:
- Add stateful user-agent and cookie handling in fake_browser
- Scrape parts in distrtibutor then part order
- Use a class inheritance approach to distributor modules, this allows
  adding state information and reduces the amount of variabled passed around.
- One scraping thread per distributor, simplify locking

Implemented in this commit:
- Used class approach to distributors and fake_browser
- Parts are scraped in distributor -> part order
- One (IO limited) python thread per distributor
- Simplified locking
@hildogjr
Copy link
Owner

hildogjr commented Jun 4, 2018

We still have to checking the Digikey module/class (I don't now if this is issue act in the others class).
Using https://github.com/xesscorp/KiCost/blob/master/tests/acquire-PWM.xml (one personal project mine when was developed the multi-part...) I realize that just have success scrape with direct part number (e.g. https://www.digikey.com/products/en?keywords=MCP4922-E%2FP-ND), part numbers that return a table (e.g. https://www.digikey.com/products/en?keywords=C0805C334K5RACTU) does not populate the spreadsheet.

@mmmaisel
Copy link
Contributor Author

mmmaisel commented Jun 5, 2018

@hildogjr The digikey issue should be fixed with my last commit.

@hildogjr hildogjr mentioned this pull request Jun 5, 2018
@hildogjr
Copy link
Owner

hildogjr commented Jun 5, 2018

Understood the change. But not fixed yet, I am debug and understanding the changes now.
acquire-PWM.xlsx

@anderwm
Copy link

anderwm commented Jun 5, 2018

I spoke to soon... with the last commit I get
No currency/country configuration for mouser.
Traceback (most recent call last):
File "/home/andersonm/kicad5_pro/KiCost/kicost/distributors/digikey/digikey.py", line 122, in dist_define_locale_currency
html = self.browser.scrape_URL(url)
File "/home/andersonm/kicad5_pro/KiCost/kicost/distributors/fake_browser.py", line 208, in scrape_URL
raise ValueError('No page')
ValueError: No page

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "/home/andersonm/kicad5_pro/ve_kicost/bin/kicost", line 11, in
load_entry_point('kicost', 'console_scripts', 'kicost')()
File "/home/andersonm/kicad5_pro/KiCost/kicost/main.py", line 274, in main
local_currency=args.currency)
File "/home/andersonm/kicad5_pro/KiCost/kicost/kicost.py", line 257, in kicost
d, instance = result.get()
File "/usr/lib/python3.6/multiprocessing/pool.py", line 644, in get
raise self._value
File "/usr/lib/python3.6/multiprocessing/pool.py", line 119, in worker
result = (True, func(*args, **kwds))
File "/home/andersonm/kicad5_pro/KiCost/kicost/kicost.py", line 244, in mt_init_dist
instance.define_locale_currency(local_currency)
File "/home/andersonm/kicad5_pro/KiCost/kicost/distributors/distributor.py", line 105, in define_locale_currency
self.dist_define_locale_currency(locale, currency)
File "/home/andersonm/kicad5_pro/KiCost/kicost/distributors/digikey/digikey.py", line 125, in dist_define_locale_currency
raise PartHtmlError
kicost.globals.PartHtmlError

Commit 5a90494 works but not for Digikey

@hildogjr
Copy link
Owner

hildogjr commented Jun 5, 2018

@anderwm by the HISTORY, currency/locale is just implemented for Digikey yet. The Digikey module is in validation now, if you just want Mouser result, you could use --include mouser or --exclude digikey for exclude Digikey.

@mmmaisel
Copy link
Contributor Author

mmmaisel commented Jun 6, 2018

@hildogjr The cause of the digikey error was that digikey detected and banned the scraper. I implemented a user-agent switch in fake_browser if KiCost is detected. This solves the digikey error and future related errors.

@anderwm I could not reproduce your error. Does this still happen with the latest version? If yes, please run KiCost with the -d3 switch to enable debugging output.

This commit finishes the refactoring.
@hildogjr
Copy link
Owner

hildogjr commented Jun 6, 2018

I am closing all the issue fix with this new branch and do some procedure to full features tests and merge.

From 558 Digikey parts, just missed 2: "5001" ("36-5001-ND") and "A-DS 09 PP/Z".
The first is a different Digikey response page (the work around is use "36-5001-ND test"), for the second part I am in doubt. But the response is now very conscious.

@hildogjr
Copy link
Owner

hildogjr commented Jun 6, 2018

Founded the source of the issue using the level 8 debug:
Looking for the "A-DS 09 PP/Z" , the link reported is https://www.digikey.com/products/en?keywords=A-DS+09+PP%2FZ+ and not https://www.digikey.com/products/en?keywords=A-DS%2009%20PP%2FZ.
Appear that the spaces (%20 code) have bing parsed as "add parameter".
To reproduce: Create a CSV file with "A-DS 09 PP/Z" and Scrape using KiCost.
Checking if there is some kind of link interpretation before the scrape.

@mmmaisel
Copy link
Contributor Author

mmmaisel commented Jun 7, 2018

@hildogjr The trailing space bug was already there before the refactoring. It was added by urlquote(pn + ' ' + extra_search_terms line even if extra_search_terms was empty.
I fixed this behaviour in the last commit.

@hildogjr
Copy link
Owner

hildogjr commented Jun 7, 2018

I will check this minor issues before release on PyPi.

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