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

Pythonise the docs #4187: "Extension types (aka. cdef classes)" (cdef_classes.rst) #4232

Merged
merged 34 commits into from Jul 14, 2021

Conversation

0dminnimda
Copy link
Contributor

@0dminnimda 0dminnimda commented Jun 17, 2021

Continuing to Pythonize Сython documentation!

This PR Pythonizes this documentation page.

Lots of process details and standardization are found here: #4187

BTW, the fact that this is a draft PR does not mean that you can no longer leave review, as I understand it, so I urge you to do it.

@0dminnimda

This comment has been minimized.

@scoder
Copy link
Contributor

scoder commented Jun 17, 2021

I think a note is best. Once readers have seen it on one page, it's easy to ignore on others. Highlighting the paragraph in a Note makes it clear that it's not part of the main page content.

@0dminnimda
Copy link
Contributor Author

0dminnimda commented Jun 19, 2021

@scoder @da-woods @Bluenix2 @mattip Faced, as I understand it, a kind of this (#1428) issue. Described the behavior here. (Not sure if there is enough difference to create a new issue)

The problem is that if people follow this tutorial step by step, they will face this behavior.
In general, the development process with pure python and using pxd is very inconvenient due to this problem.

If we want to show people pure Python code using pxd, they should definitely be warned about this issue.

It seems that this rather unpleasant problem can be solved quite simply, as far as I understand, there is already progress on its solution: #4063

Sorry for sending notifications, I just think if you can, it would be good to try to think about a solution / review the proposed PR.

@0dminnimda 0dminnimda marked this pull request as ready for review June 19, 2021 23:31
Copy link
Contributor

@da-woods da-woods left a comment

Choose a reason for hiding this comment

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

If we want to show people pure Python code using pxd, they should definitely be warned about this issue.

"Creating the .pxd file may not cause Cython to re-compile the .py file. If you are using setup.py to build you can force recompilation with the --force argument."?

It seems that this rather unpleasant problem can be solved quite simply, as far as I understand, there is already progress on its solution: #4063

I think the solution is pretty close to complete - it just needs the testing tidied up a bit. It's probably worth trying to get it merged fairly soon.

@0dminnimda
Copy link
Contributor Author

it just needs the testing tidied up a bit.

It seems that the author of that PR is not in the business of improving tests.

Oh yes, you, as a cython collaborator, can make commits in PR.

Is there anything I can do to help improve those tests?

@0dminnimda
Copy link
Contributor Author

If you are using setup.py to build you can force recompilation with the --force

Yes, it looks like a temporary solution, but it is still unpleasant to compile, for example, a large number of files every time you change one.

@0dminnimda
Copy link
Contributor Author

@scoder do you have any comments?
The only thing that might be helpful is to add a warning about #1428
Anyway, the next thing I will do after merging this PR is to solve the above issue.

@0dminnimda
Copy link
Contributor Author

@scoder #1428 is now closed so there is no need to add a warning anymore. Can this PR be merged now?

docs/examples/tutorial/cdef_classes/sin_of_square.py Outdated Show resolved Hide resolved
docs/src/tutorial/cdef_classes.rst Outdated Show resolved Hide resolved
docs/src/tutorial/cdef_classes.rst Outdated Show resolved Hide resolved
@0dminnimda
Copy link
Contributor Author

@scoder Maybe we will merge this PR?

@0dminnimda 0dminnimda closed this Jul 10, 2021
@0dminnimda 0dminnimda deleted the branch cython:master July 10, 2021 01:49
@0dminnimda 0dminnimda deleted the master branch July 10, 2021 01:49
@0dminnimda 0dminnimda restored the master branch July 10, 2021 02:17
@0dminnimda 0dminnimda reopened this Jul 10, 2021
@0dminnimda
Copy link
Contributor Author

oops experimented a little with fork branches

@0dminnimda
Copy link
Contributor Author

@scoder please merge this PR.

@scoder scoder merged commit 54fa2b8 into cython:master Jul 14, 2021
@scoder
Copy link
Contributor

scoder commented Jul 14, 2021

Thanks

@scoder scoder added this to the 3.0 milestone Jul 14, 2021
@0dminnimda
Copy link
Contributor Author

Thanks

Thanks you, finally this is merged ..

@0dminnimda 0dminnimda deleted the master branch February 25, 2022 13:31
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

3 participants