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 support for distributor TME (Issue #76) #79

Merged
merged 1 commit into from
Sep 10, 2017

Conversation

adamheinrich
Copy link
Contributor

@adamheinrich adamheinrich commented Aug 30, 2017

This PR implements #76. Contrary to other distributor implementations, the code has to do additional XmlHTTPRequests to obtain pricing information and stock availability (because the website is not loaded as a single HTML page).

This implementation does up to four HTTP requests per part. A modified implementation could reduce this to two requests (explained in a TODO comment). Is it OK to postpone this optimization to the future?

@hildogjr
Copy link
Owner

hildogjr commented Sep 2, 2017

I add you file in a new branch that I am creating https://github.com/hildogjr/KiCost/tree/restructuration (missing add in the documentation).
I intend to deal with #34 and improve the reliability of the code. I would like that you test (uncomment the line 38 in the file distributors_definitions.py in the folder distributors. In my connection, sometimes the TME got timeout.

@adamheinrich
Copy link
Contributor Author

adamheinrich commented Sep 2, 2017

Hi @hildogjr,

I add you file in a new branch that I am creating

Why?

In my connection, sometimes the TME got timeout.

I'll try to test it with some VPN. Maybe it will be enough to increase the number of attempts (HTML_RESPONSE_RETRIES). Where are you from?

@adamheinrich adamheinrich reopened this Sep 2, 2017
@hildogjr
Copy link
Owner

hildogjr commented Sep 2, 2017

A just tried to add because one of the proposed modification in my new branch is that the user do not need to modify the main kicost.py code to add new distributors or cad software. I think this would make more easy the community to add new modules (you do not need to understand) and maintenance (some people have this two different focus: software/kicad module and internet/distributor).

Also, I do not add the documentation to keep you request merge to main branch and authorship of the file when I request my merge.

I am from Brazil, now it is working. I will try to modify the number of attempts in the next time of time out, thanks by the tip.

@adamheinrich
Copy link
Contributor Author

adamheinrich commented Sep 2, 2017

Hi @hildogjr,

A just tried to add because one of the proposed modification in my new branch is that the user do not need to modify the main kicost.py code to add new distributors or cad software.

I think that's wrong. The standard approach is IMHO to add the support for TME in one PR and to do your refactoring in another PR (without the new file tme.py; I'm OK with updating it according to the new structure if the refactoring PR gets merged first).

Also, the commit history is a bit convoluted -- commit named "Files restruturation (issue #77)" adds file tme.py (which is a new feature rather than change of the file structure).

Could you please revert it? I think it's enough to remove tme.py from d88fdb8 and to remove commit 1c36070.

Signed-off-by: Adam Heinrich <adam@adamh.cz>
@adamheinrich
Copy link
Contributor Author

adamheinrich commented Sep 9, 2017

I've just updated the commit again to be in sync with the last changes in the structure.

@xesscorp xesscorp merged commit 0389e44 into hildogjr:master Sep 10, 2017
@xesscorp
Copy link
Collaborator

Adam, I merged your module into the master branch. It looks like getting part info out of TME is very complicated. Thanks for doing that! I added you to the list of authors.

I did correct a small error that appeared when running KiCost under Python 3.5: I had to decode the response from the TME URL request into a string, otherwise json.loads() would choke on it.

@adamheinrich
Copy link
Contributor Author

Hi @xesscorp, thank you and sorry for the error.

@xesscorp
Copy link
Collaborator

No problem, Adam. I do the same thing all the time. Thanks for the TME scraper!

@hildogjr
Copy link
Owner

Now that we are adding new distributors. We may add some control when call kicost able or disable some distributors and the number of retry.
We could add --retry [n] to modify HTML_RESPONSE_RETRIES and --distributors to just use the distributors that we want to scrap.
What do you think?

@adamheinrich
Copy link
Contributor Author

--distributors to just use the distributors that we want to scrap

The --include option does exactly what you want.

@hildogjr
Copy link
Owner

True, I read now.

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