Skip to content

Update codegen for API restructuring #414

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

Merged
merged 2 commits into from
May 13, 2022
Merged

Update codegen for API restructuring #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85d2701
Add back get_settings_service
mkundu1 May 13, 2022
9f74d2a
Update codegen for API restructuring
mkundu1 May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion codegen/settingsgen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ def _populate_init(parent_dir, sinfo):
os.makedirs(parent_dir)

session = launch_fluent()
sinfo = session.get_settings_service().get_static_info()
sinfo = session._settings_service.get_static_info()
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this give a warning about access to a protected member from outside the class?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's got past all the style checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another option would be for the codegen code to have a class derived from Session with a public method that invokes the protected one.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the concern in having get_settings_service() as a public method?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mkundu1 mkundu1 May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fwiw that’s where I’m at on this one

cls = flobject.get_cls("", sinfo)

_populate_hash_dict("", sinfo, cls)
Expand Down
2 changes: 1 addition & 1 deletion codegen/write_settings_yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def write_yaml(out, obj, indent=0):
print("Usage: write_settings_yaml.py [outfile]")
else:
session = pyfluent.launch_fluent()
settings = session.get_settings_service().get_obj_static_info()
settings = session._settings_service.get_obj_static_info()
if len(sys.argv) == 2:
with open(sys.argv[1], "w") as f:
write_yaml(f, settings)
Expand Down
1 change: 0 additions & 1 deletion src/ansys/fluent/core/session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ def __init__(
[("password", password)] if password else []
)
self._id = f"session-{next(Session._id_iter)}"
self._settings_root = None

if not Session._monitor_thread:
Session._monitor_thread = MonitorThread()
Expand Down