Skip to content

Drop unnecessary getattr(config, "key") calls #20578

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

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Jun 28, 2025

They're always set, no point in doing this.

(Please replace this header with a description of your pull request. Please include BOTH what you did and why you made the changes. The "why" may simply be citing a relevant Galaxy issue.)
(If fixing a bug, please add any relevant error or traceback)
(For UI components, it is recommended to include screenshots or screencasts)

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@mvdbeek mvdbeek added the kind/refactoring cleanup or refactoring of existing code, no functional changes label Jun 28, 2025
@mvdbeek mvdbeek force-pushed the drop_unncessary_getattr branch from 7473e20 to ed78e30 Compare June 28, 2025 15:30
@jmchilton
Copy link
Member

jmchilton commented Jun 28, 2025

There is a point to doing this - when code is used with alternative config objects by other libraries doing this simplifies the interface the library must produce for interacting with this code. I would push back in general but all of this seems to be from galaxy-app so I am not going to fight this but I do generally think there was a point. Especially the more useful libraries I think this made sense. For newer and more isolated code, I've switched to like local config objects with an adapter to Galaxy config object because you get stronger typing.

They're always set, no point in doing this.
@mvdbeek mvdbeek force-pushed the drop_unncessary_getattr branch from ed78e30 to b32bb64 Compare June 28, 2025 17:02
@mvdbeek
Copy link
Member Author

mvdbeek commented Jun 28, 2025

Yep, agreed, I only looked for (variants of) app.config. The typing aspect is pretty important imo, we did have bugs in the past because of typos, which are harder to catch with getattr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datatypes area/jobs area/tool-framework kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants