Skip to content
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

Update comment #91

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Update comment #91

wants to merge 5 commits into from

Conversation

2bndy5
Copy link

@2bndy5 2bndy5 commented Feb 24, 2024

resolves #90

Goals

  • reduce the visual noise in a PR thread.
  • allow only 1 comment to be posted/updated to reduce the notifications triggered from a PR sync event.

What does it do?

  • adds a new input update-comment that defaults to false (for current behavior)
  • when update-comment is set to true, the first report comment found is updated. Furthermore, if other report comments are found in the same thread, they will be deleted.

Other changes

I don't have python 3.11 installed on my system any more, so I adjusted some things to work with python 3.11+ (specifically 3.11 & 3.12).

I also prefer to use a local venv for developing on python-based projects. So, I had to add some typical names used for a local venv located at repo root to .gitignore, .flake8, and .prettierignore.

I have made sure that all unit tests still pass and added one unit test for the new functionality.

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0ca09e5) to head (13c9484).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #91   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          752       776   +24     
=========================================
+ Hits           752       776   +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@2bndy5
Copy link
Author

2bndy5 commented Mar 26, 2024

ping @per1234

api_data = self.api_request(
request="repos/" + self.repository_name + "/issues/" + str(pr_number) + "/comments",
request=request_uri,
page_number=page_number,
)

comments_data = api_data["json_data"]
for comment_data in comments_data:
# Check if the comment is a report for the PR's head SHA
if comment_data["body"].startswith(self.report_key_beginning + pr_head_sha):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only improvement I can think of is prepending self.report_key_beginning with an HTML comment:

report_key_beginning = "**Memory usage change @ "

where as using

    report_key_beginning = "<!-- arduino/report-size-deltas -->\n**Memory usage change @ "

would better assure that this action doesn't attempt to alter some user's comment starting with **Memory usage change @ .

If no comment exists, then simply post a new comment. If more than 1 comment found, then delete all but last one and update the last one.
distutils has been deprecated for years; use shutil instead

add unit test for `get_previous_comment()`
defaults to false for old behavior
update tests accordingly

replaces `get_previous_comment()` which was almost identical to `report_exists()`
update poetry lock file
@2bndy5
Copy link
Author

2bndy5 commented Sep 18, 2024

I've been keeping my fork in sync with upstream. I've also been using these changes for all nRF24 org repos (since I opened this PR), and I haven't seen any unexpected behavior so far.

Not sure what I need to do to get this reviewed (by admins). It's been idling for over half a year. This repo hasn't been getting much attention lately 😞 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update comment if it exists
3 participants