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

ensure cythonized functions are left untouched #1944

Conversation

kollmats
Copy link

Change Summary

Extending the check for untouched types to include a check for cython functions or methods.

Related issue number

Resolves #1943

@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #1944 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master     #1944   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         3909      3909           
  Branches       788       788           
=========================================
  Hits          3909      3909           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf9cc4a...06b941e. Read the comment docs.

@samuelcolvin
Copy link
Member

samuelcolvin commented Oct 9, 2020

thanks so much for the contribution.

please can you add tests and a change description.

also, in future please fill in the template in pull requests.

@samuelcolvin
Copy link
Member

bump

@samuelcolvin
Copy link
Member

@kollmats please could you fixthis.

@kollmats
Copy link
Author

kollmats commented Nov 30, 2020

Hi @samuelcolvin,

I'm sorry for this question since it may already be stated clearly somewhere, but where would be a good place to test this in your setup given that a proper test would require a cythonized application? Thanks.

@samuelcolvin
Copy link
Member

all tests are run on pydantic when compiled with cython.

Just choose the file in tests/ that makes most sense.

@PrettyWood
Copy link
Member

Made a test on #2208 just to see the CI fail with a test similar to the one in the issue and it didn't fail. I probably miss something

@samuelcolvin
Copy link
Member

I see the problem, the point is that the code where get_double_a is defined (using the example from #2208) needs to be compiled.

It doesn't matter one way or the other whether or not pydantic itself is compiled.

I'm just seeing if I can add a simple test for this.

@samuelcolvin
Copy link
Member

in the end I created another PR to fix this, see #2228. @kollmats I've thanked you in the change description, hope that is sufficient.

Thanks for this.

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.

Impl. of BaseModel with method crashes when Cythonized
3 participants