Skip to content

Restructure Fluent Connection#1671

Merged
prmukherj merged 42 commits intomainfrom
maint/restruct_fluent_connection
Jun 19, 2023
Merged

Restructure Fluent Connection#1671
prmukherj merged 42 commits intomainfrom
maint/restruct_fluent_connection

Conversation

@prmukherj
Copy link
Copy Markdown
Collaborator

@prmukherj prmukherj commented Jun 8, 2023

svar_data
should be added only to solver session classes

id
identifies a session, should be part of the base session

get_current_fluent_mode
not needed, can be a direct API call from the launcher code

start_transcript
stop_transcript
now completely redundant due to transcript.start(), transcript.stop() which are the tested methods
also, we can update the docstring in launcher.py accordingly

start_journal
stop_journal
these are general-purpose session methods; add to session base class

check_health
checks the health of the connection. Retain in fluent_connection.py

get_fluent_version
add to base session

exit
belongs in fluent_connection.py

@prmukherj prmukherj marked this pull request as ready for review June 14, 2023 09:11
@raph-luc raph-luc marked this pull request as draft June 14, 2023 13:16
@prmukherj prmukherj marked this pull request as ready for review June 15, 2023 14:52
@seanpearsonuk
Copy link
Copy Markdown
Collaborator

@seanpearsonuk @prmukherj A thought: if we are reorganizing what belongs in session vs fluent_connection, wouldn't it be a good idea to remove __dir__ and __getattr__ exposure as well? Specifically this code block:

def __getattr__(self, attr):
if attr == "root":
raise RuntimeError(
"Please use the new structure where the settings objects can be accessed directly."
" For example: 'solver.setup' or 'solver.solution'"
)
if attr == "solver":
raise RuntimeError(
"'Solver' is the parent object."
" Please use the new structure, where: session.solver => solver."
)
return getattr(self.fluent_connection, attr)
def __dir__(self):
return sorted(
set(
list(self.__dict__.keys())
+ dir(type(self))
+ dir(self.fluent_connection)

In my understanding there should be no need to expose everything under fluent_connection to session like this, particularly now that we are also going to have most functions under fluent_connection moved to or with new equivalents in session. IDE tools also don't seem to be able to deal with exposure like this very well, and documentation becomes more confusing with complete exposure like this too.

Great point, @raph-luc

@prmukherj prmukherj merged commit 54a1550 into main Jun 19, 2023
@prmukherj prmukherj deleted the maint/restruct_fluent_connection branch June 19, 2023 12:58
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.

FluentConnection class has multiple responsibilities and muddled interface

4 participants