Skip to content

Conversation

@slozier
Copy link
Contributor

@slozier slozier commented Apr 25, 2025

Follow-up to my comment in #1949.

@BCSharp
Copy link
Member

BCSharp commented Apr 25, 2025

I suppose that once this get merged, the wiki page will need to be updated as well. Or do you intend to revert to the original signature (since the whole method is now obsolete and not used in the codebase)?

@slozier
Copy link
Contributor Author

slozier commented Apr 25, 2025

I suppose that once this get merged, the wiki page will need to be updated as well. Or do you intend to revert to the original signature (since the whole method is now obsolete and not used in the codebase)?

Hmm, I guess I could revert to the original and update the wiki to say the method is obsolete and will be removed in a future release.

My main issue with the public API surface is that it's very large and it's not clear whether the APIs are public:

  1. so people can use them (e.g. something Ironclad or IronPython.SQLite would need)
  2. as an implementation detail (e. g. used by Expressions)
  3. because no one bothered to think about it

Might be interesting to create some reference assemblies with the API surface we want to support. If only I had more time... 😄

@BCSharp
Copy link
Member

BCSharp commented Apr 26, 2025

Frankly, I don't understand why there are any Call* methods in PythonOps. Why not use PythonCalls.Call? ArePythonOps.Call* used anywhere through reflection? I know that there are some public static methods defined specifically for the purpose of accessing from Expressions, we could mark the whole class hosting them as "obsolete" with a message that this is internal IronPython API, not to be used by user code (or "use at your own risk"). AFAIK, ObsoleteAttribute does not affect callability through reflection, only the compiler.

Reviewing the whole IronPython public API looks like a daunting task, that's why I didn't want to take a stance on CallWithArgsTuple lest I be tempted to start doing reviewing of stuff like that more and more. Currently I'm just trying to wrap a few loose ends before other projects saturate my availability.

@slozier
Copy link
Contributor Author

slozier commented Apr 27, 2025

Frankly, I don't understand why there are any Call* methods in PythonOps. Why not use PythonCalls.Call? ArePythonOps.Call* used anywhere through reflection? I know that there are some public static methods defined specifically for the purpose of accessing from Expressions, we could mark the whole class hosting them as "obsolete" with a message that this is internal IronPython API, not to be used by user code (or "use at your own risk"). AFAIK, ObsoleteAttribute does not affect callability through reflection, only the compiler.

Not sure why they're not going to PythonCalls.Call directly or if they're used by reflection. Maybe I'll do another PR to update the call sites to use PythonCalls directly (and then we can see if anything is still calling it). I've been marking the "internal" (reflection) stuff with EditorBrowsable(EditorBrowsableState.Never) to try and discourage use.

@slozier slozier merged commit 4758db7 into IronLanguages:main Apr 27, 2025
17 checks passed
@slozier slozier deleted the obsolete branch April 27, 2025 16:12
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