Skip to content

PyScope/PyModule API cleanup #1569

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
Merged
merged 1 commit into from
Sep 30, 2021
Merged

PyScope/PyModule API cleanup #1569

merged 1 commit into from
Sep 30, 2021

Conversation

lostmsu
Copy link
Member

@lostmsu lostmsu commented Sep 23, 2021 •
edited
Loading

  • removed PyScopeManager
  • merged PyScope into PyModule (every PyScope was a Python module anyway)
  • minor behavioral changes

Does this close any currently open issues?

This is part of 3.0 API cleanup, targeting #1390

Checklist

@lostmsu lostmsu added this to the 3.0.0 milestone Sep 24, 2021
@lostmsu
Copy link
Member Author

lostmsu commented Sep 27, 2021

@filmor can you take a look at this one at least in principle?

filmor
filmor reviewed Sep 28, 2021
View reviewed changes
@lostmsu lostmsu force-pushed the pyscope-cleanup branch 2 times, most recently from 21389d8 to 8e3cb3c Compare September 28, 2021 06:02
@lostmsu lostmsu requested a review from filmor September 28, 2021 06:06
@filmor
Copy link
Member

filmor commented Sep 30, 2021

I'm okay with this one, though for a slight backwards-compatibility I'd propose we add [Obsolete]public class PyScope : PyModule with both public constructors copied. Is there any particular reason why you renamed the file from pymodule to module? Matching the filename makes sense to me.

@lostmsu
Copy link
Member Author

lostmsu commented Sep 30, 2021 •
edited
Loading

@filmor re: filename. Without renaming the file, diff is larger (that's because the file has more code from PyScope than from PyModule). Diff tools assume pyscope is deleted, and pymodule gets tons of new stuff. When file is renamed, they properly detect that is more a rename of pyscope with minor stuff from PyModule, then a rename of pymodule with minor stuff from PyScope.

@lostmsu
Copy link
Member Author

lostmsu commented Sep 30, 2021 •
edited
Loading

@filmor re: PyScope. The original PyScope did not have any public constructors. E.g. there's no point in keeping it.

@filmor
Copy link
Member

filmor commented Sep 30, 2021

It had public static functions, though (and that's also how I always used it). So you can probably ignore the constructors, but deriving still makes sense.

@lostmsu
Copy link
Member Author

lostmsu commented Sep 30, 2021

@filmor I don't see any public static functions on old PyScope. There was a single factory method Py.Scope() and I preserved that one.

@lostmsu
PyScope/PyModule cleanup
273994b 
removed PyScopeManager
merged PyScope into PyModule
minor behavioral changes
@lostmsu lostmsu merged commit 66716db into pythonnet:master Sep 30, 2021
@lostmsu lostmsu deleted the pyscope-cleanup branch September 30, 2021 19:33
@filmor filmor mentioned this pull request Jan 18, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmor filmor filmor approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@lostmsu @filmor