Skip to content

Added docstrings and minor refactoring #1926

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

Closed
wants to merge 4 commits into from
Closed

Added docstrings and minor refactoring #1926

wants to merge 4 commits into from

Conversation

fractus
Copy link
Contributor

@fractus fractus commented Aug 29, 2022

What does this implement/fix? Explain your changes.

  • Documentation and formatting Converter.cs (max line length = 100, static member in UPPER_CASE, reorder method for readability, remove many empty lines). I didn't notice any particular guideline, so I used some common rules - no auto-formatter yet.
  • Some methods converted to private (main change in ToManagedValue now always go via the ToManaged) - I expect no performance degradation
  • Threw a few more exception when nullable

Does this close any currently open issues?

No - I started debugging a Numpy converter (as suggested in #1887) and ended up formatting this file; I will demonstrate the issue in another PR

Any other comments?

Some instruction to run the benchmarking would be useful (including the copy PythonRunTime.dll to the baseline folder; otherwise, build fails)

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
  • Ensure you have signed the .NET Foundation CLA
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

if (op == Runtime.PyLongType)
return int32Type;

if (op == Runtime.PyLongType)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block can never be reached so it's removed ; PyLongType maps to INT32_TYPE now

@lostmsu
Copy link
Member

lostmsu commented Aug 29, 2022

We will not accept any PRs with more formatting and renaming changes, than actual significant modifications to the code (e.g. new features or bug fixes).

No code should be reformatted unless it is also touched for other purposes to preserve the ability to quickly look up its nature in git blame.

@lostmsu lostmsu closed this Aug 29, 2022
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.

3 participants