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.

Sign up for GitHub

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

Jump to bottom

Incorrectly using a non-generic type with type parameters now produces a helpful Python error instead of throwing NullReferenceException #1326

Merged
merged 4 commits into from
Dec 18, 2020
Merged

Incorrectly using a non-generic type with type parameters now produces a helpful Python error instead of throwing NullReferenceException #1326

merged 4 commits into from
Dec 18, 2020

Conversation

tminka
Copy link
Contributor

@tminka tminka commented Dec 17, 2020

What does this implement/fix? Explain your changes.

Fixes #1325

Does this close any currently open issues?

Fixes #1325

Any other comments?

...

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@dnfadmin
Copy link

dnfadmin commented Dec 17, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@tminka
Copy link
Contributor Author

tminka commented Dec 17, 2020

The tests appear to be failing for the reasons addressed by PR #1260. Applying those changes should make the tests pass.

@lostmsu
Copy link
Member

lostmsu commented Dec 17, 2020

@tminka I am a bit lost. In the comment to that PR you mentioned, that de7e1ac solves the issue. But the PR itself does not contain that commit.

filmor
filmor requested changes Dec 17, 2020
View reviewed changes
Copy link
Member

@filmor filmor left a comment

Choose a reason for hiding this comment

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

Very nice addition, thank you. If you can just move the test to Python, this is good to go :)

src/embed_tests/TestList.cs Outdated Show resolved Hide resolved
src/runtime/genericutil.cs Outdated Show resolved Hide resolved
@tminka
Copy link
Contributor Author

tminka commented Dec 17, 2020

@lostmsu My only point was that the test failures are the same as the ones in the current master, and those are due to finalization/deallocation bugs.

@tminka tminka force-pushed the generics-exception branch 2 times, most recently from f2ceaf6 to 24683d0 Compare December 18, 2020 16:25
@filmor
Copy link
Member

filmor commented Dec 18, 2020

Please make this just a normal Python test. You can run pytest from within VisualStudio using our normal APIs (import pytest; pytest.main()) if you want.

@filmor
Copy link
Member

filmor commented Dec 18, 2020

Ah, I see that you have already moved the test. Would you be okay with splitting the EmbeddedPythonTest into a separate PR? I think this does have merit, but it's independent of the actual fix here.

@tminka tminka force-pushed the generics-exception branch from 5db975a to d1103ec Compare December 18, 2020 20:49
@tminka
Incorrectly using a non-generic type with type parameters now produce…
0fe042c 
…s a helpful Python error instead of throwing NullReferenceException
@tminka
Inlined GenericUtil.GenericsByName into GenericByName.
8e754b6 
Removed unused GenericUtil.GenericsForType.
Other code quality improvements.
@tminka
Moved the test to Python.
612c0cc 
Added EmbeddedPythonTest with the capability to run any desired Python tests within NUnit.
@tminka
Removed EmbeddedPythonTest
20e673f
@tminka tminka force-pushed the generics-exception branch from d1103ec to 20e673f Compare December 18, 2020 21:05
@lostmsu lostmsu merged commit d9e15a7 into pythonnet:master Dec 18, 2020
@tminka tminka deleted the generics-exception branch December 21, 2020 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lostmsu lostmsu lostmsu left review comments

@filmor filmor filmor requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NullReferenceException when non-generic type is given type parameters
4 participants
@tminka @dnfadmin @lostmsu @filmor