Skip to content
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.

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

List and discuss the name for the plugin design #496

Closed
unkcpz opened this issue Oct 6, 2023 · 5 comments · Fixed by #649
Closed

List and discuss the name for the plugin design #496

unkcpz opened this issue Oct 6, 2023 · 5 comments · Fixed by #649
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@unkcpz
Copy link
Member

unkcpz commented Oct 6, 2023

Thanks to the great work contributed by @superstar54, the plugin design allows new property developers can focus on the functionality and widgets needed for the property itself without knowledge of how other part of QeApp works.
It decouples the settings of parameters of different calculations by introducing the "Panel" as the base widget class which has some common functionality to interact with other parts of the code.
There are some terminology and methods used but not intuitively straightforward to know what is for, such as "identifier", "get_panel_value" etc.
@superstar54, can you make a list of these names and we can have a discussion with @giovannipizzi? Once we make the stable release at the end of this month, it become a new API for developers and will be hard to change after.

@unkcpz unkcpz added the enhancement New feature or request label Oct 6, 2023
@unkcpz unkcpz added this to the v2023.10.0 milestone Oct 6, 2023
@unkcpz
Copy link
Member Author

unkcpz commented Oct 22, 2023

During @superstar54's group meeting, we agreed that we release this v23.10.0 with current names for APIs, and when there are more developers trying plugin design, we discuss and fix the API in the next version.

@unkcpz unkcpz modified the milestones: v2023.10.0, v2024.4.0 Oct 22, 2023
@unkcpz
Copy link
Member Author

unkcpz commented Jan 30, 2024

Pining @AndresOrtegaGuerrero @PNOGillespie @mikibonacci who start implement the plugins, do you find some class names are not clear or bring ambiguity? If those are all fine, we can close this PR.

@mikibonacci
Copy link
Member

Hi @unkcpz, I do not have in mind examples, so for me everything is fine

@PNOGillespie
Copy link
Contributor

Hi @unkcpz, the one thing that occurs to me is that set_panel_value() in Setting wasn't entirely clear at first, which did cause a brief problem for the XAS PR because I wasn't aware that the method was intended for loading panel settings from a previous calculation.

On the one hand, the documentation for the plugin system states:

set_panel_value to tell the app how to reload the panel values from the previous calculation.

On the other hand, the docstring for set_panel_value() itself states:

"""Set a dictionary with the input parameters for the plugin."""

My suggestion is just to make sure the docstrings and documentation agree with each other, so that it's clear in both places what each part is meant to do. Changing the name of set_panel_value() might also make it clearer, but I don't know what would work here because the intended behaviour seems too specific to create a clear name for.

Other than that however, the rest of the system was clear enough for me to work with.

@unkcpz
Copy link
Member Author

unkcpz commented Feb 5, 2024

Thanks @mikibonacci @PNOGillespie for the feedback.

@superstar54 Can you keep track on this issue and make sure the advice from @PNOGillespie above is happened on the v24.04.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants