bittner / lego-mindstorms-ev3-comparison

LEGO Mindstorms EV3 Comparison
GNU General Public License v3.0
50 stars 12 forks source link

python script enhancements - fix missing Element ID when order #10

Closed bonidier closed 5 years ago

bonidier commented 6 years ago

Hi @bittner,

Following remaining part in issue #8, here is the pull request to map missing element ID with a chain of updated ID ... and more !

Regards, Didier

bonidier commented 6 years ago

Hi @bittner,

To reduce the loop, I'd moved some code outside, but not in a helper : there is too many variables to manage.

It's more readable for now.

The order() function is already long. In my mind it can be better (in a near future) to refactor the content under OOP practice, for maintainability (like using a method per step)

bittner commented 6 years ago

Wow! The order function is now huge! Can you split this up a bit?

I think such a huge function reduces readability and thus both maintainability and the probability of future contributions. We need to take care this code is in good shape. Minimum split long function bodies up into several functions, or even move large pieces that belong together in separate classes and/or modules.

I don't want to be the dog in the manger, but I only have two choices: Either oppose and continue maintaining this repo, or let this code creep into the repo and let maintenance go. :cry:

bonidier commented 6 years ago

Hi @bittner, According to our common idea, I'll split the order() function for maintenance purpose

bonidier commented 6 years ago

Hi @bittner, Here is a first try to move order() content in a module

Regards !

bittner commented 6 years ago

Not bad! Starts to look nice, doesn't it? -- (NOTE: It will be more complicated for Python beginners, because there is more than a single script to download, but that's the price for getting more features.)

Try to move the helper functions you have (the ones that start with _ and __) out into a helper module or so. And don't forget to run flake8 over your code! Gives you great feedback.

bonidier commented 5 years ago

Hi Peter, I don't forget your feedback about helper module suggestion. Here is some fixes about Lego's website evolutions since last commit

bittner commented 5 years ago

Thanks for your follow-up. Please, give me a few days for reviewing.

Thank you for your understanding.

bonidier commented 5 years ago

Hi @bittner, Thanks for your reactivity, take your time.

Just a note about survey: Lego has changed their survey form, from now we must play with HTML area tag, I don't now how to fix it, my non-working try is commented.

fortunately, the survey does not appears at each execution

bittner commented 5 years ago

I'm sorry I obviously forgot to merge these changes! Should I merge them now?

bonidier commented 5 years ago

Hi, thanks for your approval! I've just relaunched order for Home/Education sets for a last review: Here is a fix for set 45544 about missing ElementIDs to add manually (out of the set).

Now, you can merge Regards!

bittner commented 5 years ago

Sorry for the delay. I didn't get the GitHub notifications for this repo for some reason. Strange.

bittner commented 5 years ago

Thanks for your contribution! :+1: