landing_worker: add and use metadata from Revision rather than HgPatchHelper (bug 1936171) #200
Conversation
The ultimate goal of this change is to deprecate the back-and-forth use of the HgPatchHelper. This first change allows to confine the parsing of an Hg patch to the Revision object, which allows exposing cleaned-up attribute to the rest of the system.
src/lando/main/models/revision.py
Outdated
| @property | ||
| def author_name(self) -> Optional[str]: | ||
| metadata = self._parse_metadata_from_patch() | ||
| return metadata.get("author_name") | ||
|
|
||
| @property | ||
| def author_email(self) -> Optional[str]: | ||
| metadata = self._parse_metadata_from_patch() | ||
| return metadata.get("author_email") | ||
|
|
||
| @property | ||
| def commit_message(self) -> Optional[str]: | ||
| metadata = self._parse_metadata_from_patch() | ||
| return metadata.get("commit_message") | ||
|
|
||
| @property | ||
| def timestamp(self) -> Optional[str]: | ||
| metadata = self._parse_metadata_from_patch() | ||
| if ts := metadata.get("timestamp"): | ||
| # Some codepaths (via Phabricator) have the timestamp set as an int. | ||
| # We make sure it's always a string. | ||
| return str(ts) | ||
| return None |
There was a problem hiding this comment.
Feels a bit repetitive to call _parse_metadata_from_patch in each method. Maybe all these values should be determined at the same time, whenever self.patch_data is set.
There was a problem hiding this comment.
The metadata is only parsed one, and cached, as part of the _parse_metadata_from_patch() method https://github.com/mozilla-conduit/lando/pull/200/files#diff-6f00d8fe4977345e9360a87afcf4861324a3b6ff012d6a6a5716640b4b44c04dR153-R168
But calling it in every method makes sure we call it the first time we ever need any metadata.
There was a problem hiding this comment.
The repetitiveness aspect (i.e., calling _parse_metadata_from_patch in every method) was more so what I meant, not so much efficiency. I think changing _parse_metadata_from_patch to a property (e.g., _parsed_metadata_from_patch) and then using it directly in these methods would alleviate this issue.
This would become:
@property
def timestamp(self) -> Optional[str]:
if ts := self._parsed_metadata_from_patch.get("timestamp"):
# Some codepaths (via Phabricator) have the timestamp set as an int.
# We make sure it's always a string.
return str(ts)
return None| @property | ||
| def diff(self) -> str: | ||
| """Return the unified diff text without any metadata""" | ||
| # The HgPatchHelper currently returns leading newline, which we don't want to | ||
| # return here, so we strip it. | ||
| return self.patch_helper.get_diff().lstrip() | ||
|
|
||
| @property | ||
| def patch_helper(self) -> HgPatchHelper: | ||
| """Create and cache an HgPatchHelper to parse the raw patch with Hg metadata.""" | ||
| if not self._patch_helper: | ||
| self._patch_helper = HgPatchHelper(StringIO(self.patch)) | ||
| if not self._patch_helper.diff_start_line: | ||
| raise NoDiffStartLine | ||
|
|
||
| return self._patch_helper |
There was a problem hiding this comment.
These two methods are hg-specific, wonder if we should be making them more scm-agnostic?
There was a problem hiding this comment.
Yes, the purpose of this change is a stop gap to start hiding the HgPatchHelper.
Historically, we transformed Phab revision into a Hg Patch, which we no longer need to do.
In this first cut, we hide the patch helper from everything else, and let the interface of the Revision be sufficient, and we can change the implementation later on.
shtrom
dismissed
zzzeid’s stale review
@Property added; I don't think support for a GitPatchHelper is needed for now.
The ultimate goal of this change is to deprecate the back-and-forth use of the HgPatchHelper. This first change allows to confine the parsing of an Hg patch to the Revision object, which allows exposing cleaned-up attribute to the rest of the system.