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

Fix invalid assert in exporters #3574

Merged
merged 1 commit into from
Jan 26, 2017
Merged

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented Jan 12, 2017

Assert that the length is greater than one rather than the value itself. This bug was introduced in the commit:
329be06 -
"exporters - group by directories in prj root"

Assert that the length is greater than one rather than the value
itself. This bug was introduced in the commit:
329be06 -
"exporters - group by directories in prj root"
@c1728p9
Copy link
Contributor Author

c1728p9 commented Jan 12, 2017

/morph export-build

@sarahmarshy
Copy link
Contributor

We might want to avoid a runtime traceback and fail gracefully with an exception + message. mbed-cli should catch any raised exceptions. @theotherjimmy, what do you think?

@theotherjimmy
Copy link
Contributor

Just use NotSupportedException with a human readable reason.

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 79

All exports and builds passed!

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 13, 2017

@c1728p9 please update as suggested above

@c1728p9
Copy link
Contributor Author

c1728p9 commented Jan 13, 2017

This assert is meant to catch logic errors, so it shouldn't occur in practice. If you are concerned about it propagating as a possible runtime error that isn't a bug in the code, it should probably just be handled directly - something like changing this:

assert len(path_list) >= 1
if len(path_list) == 1:
    key = self.project_name
else:
    key = path_list[0]

to this:

if len(path_list) > 1:
    key = path_list[0]
else:
    key = self.project_name

That way even if an empty list is returned an exception doesn't occur. What are your thoughts?

@bridadan
Copy link
Contributor

@c1728p9 seems reasonable to me. How about you @sarahmarshy?

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 16, 2017

@sarahmarshy @theotherjimmy What do you think about the proposal above?

@theotherjimmy
Copy link
Contributor

@c1728p9 I gave it a 👍

@bridadan bridadan self-requested a review January 17, 2017 20:01
@bridadan
Copy link
Contributor

To wrap up the assert vs Exception debate, I think this is fine to come in as is. And if we feel strongly about "no asserts in the tools", then we should have another PR that replaces all the existing asserts in the tools with exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants