chore: Perform deprecation call from generated data. #4162
Conversation
prmukherj
requested review from
mkundu1,
seanpearsonuk and
hpohekar
as code owners
| deprecated_class_version.update({cls_name: deprecated}) | ||
| release_version = "20" + cls._deprecated_version.replace(".", "R") | ||
| r.write(f".. deprecated:: Ansys {release_version}\n\n") | ||
| deprecated_class_version.update({cls_name: release_version}) |
There was a problem hiding this comment.
@prmukherj
This function is getting increasingly complex and hard to follow, especially with the addition of edge-case handling like the if deprecated: block. A few concerns:
-
It’s now doing too much. We should consider decomposing it into smaller, well-named helper functions for clarity and testability.
-
The magic string logic for converting versions (e.g.,
replace(".", "R")) is brittle and obscure. This logic belongs in theFluentVersionclass, where it can be centrally maintained and tested. -
Some parts (e.g.,
cls_orig_name == "root"and hardcoded RST section generation) would benefit from explanatory comments. -
As the doc generation grows, we need to ensure we’re not making the code harder for others (or future us) to safely maintain.
Could we take a moment to refactor this function as part of this PR, or otherwise track it for near-term cleanup?
There was a problem hiding this comment.
I strongly agree with all your points and we had a discussion regarding this as well with @mkundu1. Actually it was an already existing code and as part of this PR we just moved it. There will be another part to this PR that will contain some refactoring changes, for which @mkundu1 has already created a task. I'll address this as well then.
Thank you for your suggestions.
| """Whether the object is deprecated in a specific Fluent version.""" | ||
| deprecated_version = getattr(obj, "_deprecated_version", None) | ||
| return deprecated_version and ( | ||
| float(deprecated_version) <= 22.2 |
There was a problem hiding this comment.
@prmukherj Should this comparison go via the FluentVersion class? Is it possible? (It might require FluentVersion to be updated methinks.)
There was a problem hiding this comment.
yes, possible. Will do it.
Perform deprecation call from generated data.