-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
make makeqstrdata.py more pythonic #481
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
Conversation
replace list-of-tuples with a dict
Well, actually, I wanted it to be a list (not a dict) because then I can be sure of the order in which the regex's are checked. In a more sophisticated scenario (where I pulled this idiom from), multiple regexs can match, and I put the most specific (ie less generic) at the start of the list. |
so that regexes always get checked in correct order
Better? :) |
I honestly don't see what's wrong with using a list of pairs: it's simpler and obvious what it's doing. |
Fine! Closing. |
Actually, I did like that you assigned the regex's to a variable, instead of having them inline. |
@lurch: There's good programmer's saying: if it didn't break, don't fix it ;-). I don't want to discourage refactoring and clean up, but there should be clear gradient in such activity. Here I agree with @dpgeorge that it's hard to see it. (If tomorrow someone else would come submitting opposite changes, calling them "more pythonic", it would be hard to respond something definitive either; and there rather not be such situation like flip-flopping code). |
@lurch please don't take offence :) It's done all in the name of good code. With more people following uPy and using it, we need to be concise and direct with what we do. |
In cases like this where the original code is done a particular way, but it isn't obvious why it was done that way, I find that adding a comment for future code readers to be useful. |
Well, the first thing I did was to pull out the ugly inline-list-of-tuples into a separate variable (with the view that it would make the code more maintainable should additional regexes need to be added later), and then I just thought "this looks very much like a dict, so I'll just turn it into a dict". It wasn't until @dpgeorge's comment where he said that some regexes might overlap, that it became obvious that the order mattered. Yes, I agree that with more people (trying) to help out with uPy, that it's important to be selective and keep code quality high, and not just blindly accept all pull requests. |
/docs/design_guide: added links to firmware build learning guides for SAMD21 & ESP8266. Changes were placed in the "Adding native modules" section, since that seemed to me the best place based on target audience. Updated documentation for `delay()` which fixes micropython#243.
replace list-of-tuples with a dict