Skip to content

Cleanup use of dicts, e.g. use of .keys()#5165

Open
Flamefire wants to merge 2 commits into
easybuilders:developfrom
Flamefire:cleanup-dicts
Open

Cleanup use of dicts, e.g. use of .keys()#5165
Flamefire wants to merge 2 commits into
easybuilders:developfrom
Flamefire:cleanup-dicts

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

Using x in dict.keys() is discouraged and should be x in dict directly which is potentially more performant and easier to read/shorter. Accesses to keys and their values should use dict.items to avoid the lookup overhead.

@Flamefire Flamefire force-pushed the cleanup-dicts branch 2 times, most recently from efc69aa to e3683a6 Compare April 10, 2026 15:25
Using `x in dict.keys()` is discouraged and should be `x in dict`
directly which is potentially more performant and easier to read/shorter.
Accesses to keys and their values should use `dict.items` to avoid the
lookup overhead.
Comment thread easybuild/tools/github.py Outdated
_log.info("Trying to derive target repository based on specified files...")

easyconfigs, files_to_delete, patch_files, py_files = [paths[key] for key in sorted(paths.keys())]
easyconfigs, files_to_delete, patch_files, py_files = [item[1] for item in sorted(paths.items())]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could this not be simplified to the following?

Suggested change
easyconfigs, files_to_delete, patch_files, py_files = [item[1] for item in sorted(paths.items())]
easyconfigs, files_to_delete, patch_files, py_files = sorted(paths.values())

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need to sort by key so that we get the right order.
It looks like a dataclass or NamedTuple would be better than a dict where you know all existing keys.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah yes, of course

Copy link
Copy Markdown
Contributor Author

@Flamefire Flamefire May 7, 2026

Choose a reason for hiding this comment

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

TBH: I had to read the code here and the caller again to understand this. Shall I add a commit changing this to a named Tuple? Even though it is a "public" method I'd doubt anyone would be using this outside of us.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would be fine with that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That bubbled up the callchain more than I expected: The origin of this dict is categorize_files_by_type
IMO still very much worth it: Makes it much easier to reason about what is actually passed instead of having a paths parameter in multiple places and sometimes it is a list of paths, and sometimes a dict with only some keys which might be misspelled.

If you'd like to review that change in isolation then use #5190 and I'll rebase this (will cause an easy conflict to resolve)

@jfgrimm jfgrimm added this to the 5.x milestone May 7, 2026
The current dict is pretty opaque and makes spelling mistakes easy.
Avoid by using a typed, named tuple.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants