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

Fixes #688 by Handling null _vars #703

Merged
merged 3 commits into from
Aug 21, 2015
Merged

Fixes #688 by Handling null _vars #703

merged 3 commits into from
Aug 21, 2015

Conversation

crwilcox
Copy link
Contributor

@crwilcox crwilcox commented Aug 7, 2015

_vars was being set to null in at least on spot (ModuleAnalysis 455).
Now it is set as an empty list in cases where it could come as null.

_vars was being set to null in at least on spot (ModuleAnalysis 455).
Now it is set as an empty list in cases where it could come as null.
@crwilcox
Copy link
Contributor Author

crwilcox commented Aug 7, 2015

I am working on adding a test for this yet, but this is my proposed patch.

@int19h
Copy link
Contributor

int19h commented Aug 7, 2015

Are we sure that this is the only place where it's not handling null? Should the fix rather be removing that one single case where null is used, and replacing it with an empty collection? It looks like the code tries really hard to avoid nulls elsewhere.

@crwilcox
Copy link
Contributor Author

crwilcox commented Aug 7, 2015

I considered doing that also. These are all of the constructors. Granted, you are correct that we could still do it incorrectly. Any thoughts on doing both?

@int19h
Copy link
Contributor

int19h commented Aug 7, 2015

Never mind, having had a look at the actual code diff I'm actually fine with either.

@int19h
Copy link
Contributor

int19h commented Aug 7, 2015

👍

@@ -451,8 +451,8 @@ private class ErrorWalker : PythonWalker {
.Select(s => new MemberResult(s.Name, s.GetMergedAnalysisValues()))
.ToList();
} catch (Exception) {
// TODO: log exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO should stay here, though we probably need to let the exception propagate to be able to log it from outside Analysis.dll.

@zooba
Copy link
Member

zooba commented Aug 8, 2015

Go ahead and merge after adding the comment back.

crwilcox added a commit that referenced this pull request Aug 21, 2015
Fixes #688 by Handling null _vars
@crwilcox crwilcox merged commit a9d1120 into microsoft:master Aug 21, 2015
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.

None yet

4 participants