Add the options data class to program #237
Conversation
leofang
added
P1
Thanks, Berry 🙏 We welcome feedbacks and won't ignore anyone! |
|
Another TODO: Let's also update the code samples and show the best practice! |
Is there a way to add these as doctests so that the code samples must always be kept in working order? |
|
@warsawnv Totally. I'm actively working on getting github actions working, and the examples are invoked as part of the testsuite. |
|
/ok to test |
|
Very good point |
ksimpson-work
force-pushed
the
ksimpson/add_program_options
branch
from
03128c8 to
6d789cb
Compare
…to ksimpson/add_program_options
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
last few comments addressed, tests improved and passing |
|
I still see many review comments above not addressed? |
|
one was an optional suggestion and one was a question. I didn't want to makr as resolved so you had a chance to see my response, but there weren't associated code changes |
Oh it's more than that 😛 You need to scroll up and unfold all the hidden comments (GH folds them if too many):
|
…to ksimpson/add_program_options
… error number options
|
Thanks fcor pointing that out, and for the very in depth review which helped me find a couple issues as well as general improvements. I believe everything has now been addressed /ok to test |
|
/ok to test |
| """ | ||
| Check if the given object is a sequence (list or tuple). | ||
| """ | ||
| return isinstance(obj, (list, tuple)) |
There was a problem hiding this comment.
Q: Couldn't we just use isinstance(obj, Sequence) here?
There was a problem hiding this comment.
no because a string is a sequence. That's what I originally wanted to do
There was a problem hiding this comment.
This was why I left #237 (comment) and #237 (comment) above. str is a nested sequence by our definition, because any of its elements is also a str. If we check isinstance(something, str) first to special case it, the remaining checks can be simplified and not specialized for any known container (list/tuple).
| """ | ||
| Check if the given object is a nested sequence (list or tuple with atleast one list or tuple element). | ||
| """ | ||
| return is_list_or_tuple(obj) and any(is_list_or_tuple(elem) for elem in obj) |
There was a problem hiding this comment.
ditto, also, it should be all:
| return is_list_or_tuple(obj) and any(is_list_or_tuple(elem) for elem in obj) | |
| return is_list_or_tuple(obj) and all(is_list_or_tuple(elem) for elem in obj) |
There was a problem hiding this comment.
I think perhaps I should take this out of utils and leave it in program if it is too specific, but to handle the case in question it should be any imo, since I would like to support define_macro = ("MY_MACRO", ("MY_OTHER_MACRO", 99))
close #221
Add the options class to Program.
This comes with a few side effects, namely: There are some modifications to the tests, and the LinkerOptions class now accepts None for arch (using the current device as a default), for consistency between the program options and linker options