Update codegen for API restructuring #414
Conversation
mkundu1
requested review from
ajain-work,
dnwillia-work,
lagacep-ans and
seanpearsonuk
src/ansys/fluent/core/session.py
Outdated
| ) | ||
| self.solver = Session.Solver( | ||
| self._datamodel_service_tui, self._settings_service | ||
| self._datamodel_service_tui, self.get_settings_service() |
There was a problem hiding this comment.
I would prefer if codegen called _get_settings_service() so that the method would be identified as private for library users. What do you think?
There was a problem hiding this comment.
Yeah, we don't need to expose the services from the session object. Actually the codegen can just use the private attribute session._settings_service.
There was a problem hiding this comment.
Yeah, we don't need to expose the services from the session object. Actually the codegen can just use the private attribute
session._settings_service.
Great, I'll approve on that basis. Thanks.
mkundu1
changed the title
|
|
||
| session = launch_fluent() | ||
| sinfo = session.get_settings_service().get_static_info() | ||
| sinfo = session._settings_service.get_static_info() |
There was a problem hiding this comment.
Wouldn't this give a warning about access to a protected member from outside the class?
There was a problem hiding this comment.
It's got past all the style checks.
There was a problem hiding this comment.
That is because we skip style checks for the settings directory. But from a maintainer of the session class point of view, I would assume that _settings_service is not being used outside my class.
There was a problem hiding this comment.
Another option would be for the codegen code to have a class derived from Session with a public method that invokes the protected one.
There was a problem hiding this comment.
What is the concern in having get_settings_service() as a public method?
There was a problem hiding this comment.
My assumption was that it was redundant as a member of the public API. Is that wrong? I didn't look into it, but assumed.
There was a problem hiding this comment.
I feel if we decide to expose the services from the session instance as public API (for consumption by pyfluent users), we should do that uniformly for all services.
I think the session class's maintainer's responsibility is upto marking the members appropriately as public or non-public and documenting the public API. The client code is responsible for the usage of the non-public API and if the client code breaks when that part of the API is changed unannounced (https://docs.python-guide.org/writing/style/#we-are-all-responsible-users). In the current case, the client code is used by developers and is not packaged for users, so I think usage of the non-public API is ok. If we decide to make codegen module more public in future, we can expose the services from session instance, or the codegen module can directly use the classes like SettingsService from the services module.
There was a problem hiding this comment.
Fwiw that’s where I’m at on this one
5 participants
No description provided.