add an API to retrieve an enum's name property

[leiflm]

Enums may have a name that is typically displayed to the user. This adds an API to allow access to that.

[leiflm]

do you need me to do anything else here?

[leelavg]

possible to force push once? not sure why the CI failed.

[leiflm]

possible to force push once? not sure why the CI failed.

It is not, since the state I'd push would be the exact same I already pushed.

Supposedly it's not possible to re-run jobs after 30 days. But maybe it is for an authorized contributor?

[waynesun09]

Review Summary

Thanks for the contribution — the intent is solid. Having an API to resolve an enum value to its display name is a natural complement to get_valid_field_values. However, there are a couple of bugs that need fixing before merge, plus some improvements worth considering.

---

Bugs (must fix)

1. Broken f-string interpolation in error message

The second string literal is missing the f prefix, so {value} renders literally instead of being interpolated. There's also a missing space before "or":

# Current (broken):
raise PyleroLibException(
        f'The enum element "{enum_id}"'
        'or its value "{value}" do not exist.'
    )

# Fixed:
raise PyleroLibException(
        f'The enum element "{enum_id}" '
        f'or its value "{value}" do not exist.'
    )

2. Inconsistent indentation in the for loop body

The if statement uses 2-space indentation (10 spaces total) instead of the project-standard 4-space increment (12 spaces total). This will likely fail the pre-commit linter checks:

# Current (wrong):
        for enum in enums:
          if enum.id == value:   # 2-space indent
                return enum.name

# Fixed:
        for enum in enums:
            if enum.id == value:   # 4-space indent
                return enum.name

(This is likely why the CI "Code Quality (3.11)" job failed on the original run.)

---

Warnings (should fix)

3. Docstring is inaccurate — The description "Gets the available enumeration options" is copied from get_valid_field_values. This method resolves a single value to its display name. Suggested:

"""Resolves an enumeration value to its display name.
...
Returns:
    The display name (enum.name) for the given value

Raises:
    PyleroLibException: If no option with the given value exists
"""

4. Code duplication — The entire cache-lookup block is copy-pasted from get_valid_field_values. Consider extracting a shared helper:

def _get_enum_options(self, enum_id, control=None):
    """Fetch enum options for the given id, using cache."""
    project_id = getattr(self, "project_id", None) or self.default_project
    enum_base = self._cache["enums"].get(enum_id)
    enums = None
    if enum_base:
        enums = enum_base.get(control)
    if not enums:
        enums = self.session.tracker_client.service.getEnumOptionsForIdWithControl(
            project_id, enum_id, control
        )
        if enum_id not in self._cache["enums"]:
            self._cache["enums"][enum_id] = {}
        self._cache["enums"][enum_id][control] = enums
    return enums

Then both methods become thin wrappers — easier to maintain and test.

5. Cache overwrite bug (pre-existing) — self._cache["enums"][enum_id] = {} wipes previously cached control keys for that enum_id. Should be:

if enum_id not in self._cache["enums"]:
    self._cache["enums"][enum_id] = {}
self._cache["enums"][enum_id][control] = enums

This is inherited from get_valid_field_values, but since you're touching this code it's a good opportunity to fix.

6. No tests — Consider adding tests for: happy path (value exists → name returned), not-found (raises PyleroLibException), and cache behavior.

---

Nits

7. Error message could be clearer — By the time the exception is raised, we know the enum resolved (otherwise the WSDL call would have failed). Only the value was not found. A clearer message:

f'Value "{value}" not found in enum "{enum_id}" (control={control!r})'

[waynesun09]

Inline comments for findings 1-4. See the earlier review for the full summary.

[waynesun09]

Warning: Code duplication — consider extracting a shared helper

This entire cache-lookup block is duplicated verbatim from get_valid_field_values. If the caching logic ever changes, both methods must be updated in lockstep.

Consider extracting a private helper:

def _get_enum_options(self, enum_id, control=None):
    """Fetch enum options for the given id, using cache."""
    project_id = getattr(self, "project_id", None) or self.default_project
    enum_base = self._cache["enums"].get(enum_id)
    enums = None
    if enum_base:
        enums = enum_base.get(control)
    if not enums:
        enums = self.session.tracker_client.service.getEnumOptionsForIdWithControl(
            project_id, enum_id, control
        )
        if enum_id not in self._cache["enums"]:
            self._cache["enums"][enum_id] = {}
        self._cache["enums"][enum_id][control] = enums
    return enums

Then both methods become thin wrappers:

def get_valid_field_values(self, enum_id, control=None):
    return [enum.id for enum in self._get_enum_options(enum_id, control)]

def get_name_for_field_value(self, enum_id, value, control=None):
    for enum in self._get_enum_options(enum_id, control):
        if enum.id == value:
            return enum.name
    raise PyleroLibException(...)

Note: the helper also fixes a pre-existing cache bug — self._cache["enums"][enum_id] = {} wipes other cached control keys for the same enum_id. Using if enum_id not in avoids that.

[waynesun09]

Re-review of updated changes

Thanks for addressing the feedback — the refactoring looks great. All critical issues from the first review are resolved:

Previous Feedback	Status
Broken f-string (missing f prefix)	Fixed
Inconsistent 2-space indentation	Fixed
Inaccurate docstring	Fixed
Extract shared _get_enum_options helper	Fixed — clean extraction
Cache overwrite bug	Fixed (if enum_id not in)

The refactoring is correct — get_valid_field_values preserves identical behavior, and the _get_enum_options helper is a clean abstraction.

---

CI is failing — ruff-format

The pre-commit ruff-format hook is reformatting two spots. Please run pre-commit run -a locally and push the fix:

1. The getEnumOptionsForIdWithControl(...) call — ruff wants it on one line (fits within 120 chars)
2. The raise PyleroLibException(...) — ruff wants it on one line

---

Remaining low-severity items (non-blocking)

Error message could be clearer — By the time the exception is raised, the enum resolved successfully; only the value wasn't found. Current message implies both might be invalid. Consider:

f'Value "{value}" not found in enum "{enum_id}"'

get_valid_field_values docstring — It says Returns: Array of EnumOptions but actually returns a list of string IDs. Natural time to fix since docstrings were already touched.

No tests — Not a merge blocker, but a unit test for get_name_for_field_value (happy path + not-found) with a mocked _get_enum_options would be easy to add.

[waynesun09]

LGTM. The refactoring is clean, get_valid_field_values preserves identical behavior, the cache overwrite bug fix is a nice improvement, and CI passes.

Minor non-blocking suggestions for a follow-up:

• Error message could be more precise (the enum resolved; only the value wasn't found)
• Cache docstring is now duplicated across all three methods — could be trimmed from the two public ones
• Unit tests for get_name_for_field_value would be a good addition