Skip to content

Replace raw dict by CategorizedPaths object (NamedTuple(-like)) - #5269

Open
Flamefire wants to merge 6 commits into
easybuilders:developfrom
Flamefire:categorized-paths
Open

Flamefire wants to merge 6 commits into
easybuilders:developfrom
Flamefire:categorized-paths

Conversation

@Flamefire

Copy link
Copy Markdown
Contributor

The current dict is pretty opaque and makes spelling mistakes easy.
Avoid by using a typed, named tuple.

There should be no other entries as this is mostly an internal class but the backwards-compat path allows it for now.

In EB 6.0 this can be a pure NamedTuple or a dataclass

@Flamefire
Flamefire force-pushed the categorized-paths branch 2 times, most recently from b4f3052 to 3430b8d Compare September 4, 2026 13:14
@boegel

boegel commented Sep 9, 2026

Copy link
Copy Markdown
Member

In general, I like this, it indeed leaves less room for mistakes.

However, we're changing the function signature and or return type of several functions that are part of our API here:

  • categorize_files_by_type now returns a CategorizedPaths instance, instead of a dict;
  • new_branch_github, new_pr_from_branch, new_pr, det_pr_target_repo, update_branch, update_pr now take a CategorizedPaths instance as a value, rather than the dict they used to take before;

Even if we wait with merging this for EasyBuild 6.0, we should still deprecate the old function signatures.

This could be done by:

  • adding an option to categorize_files_by_type to determine the type of return value (dict by default, opt-in to CategorizedPaths instance);
  • auto-convert a dict value passed to new_pr & co to a CategorizedPaths instance internally, if needed (so accept both dict and CategorizedPaths instance as input value);

I agree that it's unlikely that people would be using one of these functions, but if anyone does (for example in a Python script that leverages EasyBuild framework as a library, see also here), then we would be breaking their script.

Backwards compatibility is a strong focus point of EasyBuild, and I very much want to keep it like that...

@boegel boegel added this to the 5.x milestone Sep 9, 2026
@boegel boegel changed the title Replace raw dict by CategorizedPaths NamedTuple(-like) Replace raw dict by CategorizedPaths object (NamedTuple(-like)) Sep 9, 2026
Comment thread easybuild/tools/github.py Outdated
_log.warning("Failed to import 'git' Python module: %s", err)


class CategorizedPaths(MutableMapping): # Todo: use NamedTuple or dataclass instead

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.

Why not use NamedTuple already? It probably requires a newer Python version as minimal version?
If so, mention that in the comment (and put it above the class line while you're at it)

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 the dict-interface. Without that the NamedTuple (or dataclass in 3.9) from 4a79421 are enough:

class CategorizedPaths(NamedTuple):
    easyconfigs: List[str]
    easyconfigs: List[str]
    files_to_delete: List[str]
    patch_files: List[str]
    py_files: List[str]

Enhanced the comment and added another one below

@Flamefire

Copy link
Copy Markdown
Contributor Author

This could be done by:

* adding an option to `categorize_files_by_type` to determine the type of return value (`dict` by default, opt-in to `CategorizedPaths` instance);

* auto-convert a `dict` value passed to `new_pr` & co to a `CategorizedPaths` instance internally, if needed (so accept both `dict` and `CategorizedPaths` instance as input value);

Is that an either-or or both?
I would have said the first is enough but we can be extra save and do both. Now commit for the 2nd point added

The current dict is pretty opaque and makes spelling mistakes easy.
Avoid by using a typed, named tuple.
The test wrongly matched:
> 'gzip' unexpectedly found in '== Temporary log file in case of crash /tmp/eb-2gzipb3w/eb-o3rmuvme/eb-p0oq22mq/eb-test-9go8uv1x.log

i.e. it matched the path.

Use whole-word regex to avoid the short string.
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