-
Notifications
You must be signed in to change notification settings - Fork 0
#171 - Introduce new records ID #173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a3f79a4
14a52cd
6875a1f
7bc48b1
31d244e
858e146
a96aac8
384dbee
572f333
4503687
e57d7fe
e941880
24ab91b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,8 +23,8 @@ def __init__(self, issue: Issue, issue_labels: Optional[list[str]] = None, skip: | |
super().__init__(issue, issue_labels, skip) | ||
|
||
self._level: int = 0 | ||
self._sub_issues: dict[int, SubIssueRecord] = {} | ||
self._sub_hierarchy_issues: dict[int, "HierarchyIssueRecord"] = {} | ||
self._sub_issues: dict[str, SubIssueRecord] = {} | ||
self._sub_hierarchy_issues: dict[str, "HierarchyIssueRecord"] = {} | ||
|
||
@property | ||
def level(self) -> int: | ||
|
@@ -120,7 +120,8 @@ def to_chapter_row(self, add_into_chapters: bool = True) -> str: | |
|
||
# add sub-hierarchy issues | ||
for sub_hierarchy_issue in self._sub_hierarchy_issues.values(): | ||
row = f"{row}\n{sub_hierarchy_issue.to_chapter_row()}" | ||
if sub_hierarchy_issue.contains_change_increment(): | ||
row = f"{row}\n{sub_hierarchy_issue.to_chapter_row()}" | ||
|
||
# add sub-issues | ||
if len(self._sub_issues) > 0: | ||
|
@@ -129,6 +130,9 @@ def to_chapter_row(self, add_into_chapters: bool = True) -> str: | |
if sub_issue.is_open: | ||
continue # only closed issues are reported in release notes | ||
|
||
if not sub_issue.contains_change_increment(): | ||
continue # skip sub-issues without change increment | ||
Comment on lines
+133
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don’t drop cross-repo sub-issues when rendering. With the new cross-repo detection, sub-issues from external repositories generally won’t carry local PRs or commits, so 🤖 Prompt for AI Agents
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check validity of this review note. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainBased on my analysis of the codebase, I can now provide a comprehensive response about the validity of CodeRabbit's review comment. @miroslavpojer The CodeRabbit review comment appears to be valid and identifies a legitimate issue. Here's my analysis: Validity Assessment: ✅ VALIDKey findings:
RecommendationThe review suggestion is sound. You should modify the conditional to allow cross-repo sub-issues to render even when they lack local change increments. Consider one of these approaches:
This is a critical issue that would prevent the core functionality introduced in this PR from working as intended. 🏁 Script executed:
Length of output: 10207 🏁 Script executed:
Length of output: 13624 🏁 Script executed:
Length of output: 5242 🏁 Script executed:
Length of output: 2085 🏁 Script executed:
Length of output: 181 🏁 Script executed:
Length of output: 1247 @miroslavpojer The CodeRabbit review comment is ✅ VALID and identifies a critical issue that would prevent the core functionality of this PR from working correctly. Analysis SummaryThe Problem:
Evidence from Code AnalysisFrom the factory code (lines 98-103 in # Third register all external sub-issues
for ext_sub_issue in self.__external_sub_issues:
if self.get_id(ext_sub_issue) in self.__registered_issues:
continue
self._create_record_for_sub_issue(ext_sub_issue) # Creates SubIssueRecord These external sub-issues won't have local PRs, so RecommendationYou need to modify the filtering logic to allow cross-repo sub-issues to render. Consider:
This is a high-priority issue that would make the cross-repo sub-issue feature non-functional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see it will be solved in next issue, see new TODO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@miroslavpojer I understand you're planning to address this in a future issue as indicated by the TODO. This is a reasonable approach given that the cross-repo sub-issue filtering issue is a significant design consideration that might require broader changes. The CodeRabbit review correctly identified that the current 🐰 (・ω・) 🏁 Script executed:
Length of output: 206 🏁 Script executed:
Length of output: 2019 @miroslavpojer I understand your approach to defer this cross-repo sub-issue filtering issue to a future iteration. While I wasn't able to locate the specific TODO you're referencing in the current codebase, I trust that you have a clear plan for addressing this in a subsequent issue. This is actually a wise approach given the complexity involved - properly handling cross-repo sub-issues in the rendering pipeline requires careful consideration of:
The CodeRabbit review correctly identified the issue, but addressing it properly likely requires broader architectural decisions that are better suited for dedicated issue planning rather than a quick fix in this PR. Your current implementation successfully introduces the core cross-repo detection and collection functionality (the main objectives of issues #171 and #172), and the rendering refinement can be addressed in the follow-up work. 🐰 (・ω・) 🧠 Learnings used
|
||
|
||
sub_issue_block = "- " + sub_issue.to_chapter_row() | ||
ind_child_block = "\n".join( | ||
f"{sub_indent}{line}" if line else "" for line in sub_issue_block.splitlines() | ||
|
Uh oh!
There was an error while loading. Please reload this page.