Skip to content

fixing VS solution - compilation symbol is used for detect which interop**.cs to use#169

Closed
den-run-ai wants to merge 9 commits intopythonnet:developfrom
den-run-ai:patch-5
Closed

fixing VS solution - compilation symbol is used for detect which interop**.cs to use#169
den-run-ai wants to merge 9 commits intopythonnet:developfrom
den-run-ai:patch-5

Conversation

@den-run-ai
Copy link
Copy Markdown
Contributor

patches geninterop.py to output compilation symbon conditional, includes all interop**.cs to solution

@tonyroberts
Copy link
Copy Markdown
Contributor

This PR doesn't meet the guidelines set out in contributing.md. In addition, this will not work when the abi flags are different (eg a debug build of Python). The solution of doing a string replace seems inelegant to me. Commenting out code the should be deleted is something I've mentioned before I think; I'll add a point about that to the contrib file.

@den-run-ai
Copy link
Copy Markdown
Contributor Author

  1. Can you please point out which guidelines are violated?
  2. Can you explain in more details how abi flags can get different?
  3. I can improve string manipulation - so use formatting only? I did not want to mess with your formatting using old python syntax %.
  4. I can remove the commented out line from setup.py.

BTW, the clrmodule project (which compiles to clr.pyd) does not have synchronized python version flag with Python.Runtime.DLL. Not sure how only one flag can be shared between 2 projects?

@tonyroberts
Copy link
Copy Markdown
Contributor

  1. You've been told countless times about grouping files into logical units. I'm getting pretty tired of telling you.
  2. This information is already available online.
  3. If you didn't want to mess with it then continue to use the old style formatting (noting that this code has to run with older versions of Python).
  4. That code is still needed, as you will see once you take the time to understand it.

I'm closing this PR. I will do the necessary changes myself when I have time.

@tonyroberts
Copy link
Copy Markdown
Contributor

See PR #171.

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.

2 participants