Improve refresh code excerpts when checking codes #9194

AlexV525 · 2023-08-05T07:38:49Z

Detecting code excerpts during checking codes would not tell what codes are refreshed during the check. This PR adds git diff result to tell the difference.

This was inspired during our sync work and implemented in cfug/flutter.cn#1341.

Presubmit checklist

This PR doesn’t contain automatically generated corrections (Grammarly or similar).
This PR follows the Google Developer Documentation Style Guidelines — for example, it doesn’t use i.e. or e.g., and it avoids I and we (first person).
This PR uses semantic line breaks of 80 characters or fewer.

github-actions · 2023-08-09T21:15:35Z

Visit the preview URL for this PR (updated for commit a157999):

https://flutter-docs-prod--pr9194-feat-improve-refresh-tupn2fio.web.app

_{(expires Thu, 02 Nov 2023 09:44:01 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: d5ba327eec813901cac8396c4f458b02288624ab}

AlexV525 · 2023-08-17T16:38:03Z

@parlough WDYT about this one? We can also update the code_excerpt_updater tool to output changes. But git diff might be more visual.

parlough · 2023-08-18T02:17:28Z

Sorry that I haven't taken a look at this yet. I'm working through a long list of reviews and other work.

Without taking a look, adding the support into the tool itself sounds more flexible, but using git diff might be a good solution for now. Thanks for working on this and I'll make sure to take a look as soon as I can :D

AlexV525 · 2023-09-17T06:07:08Z

ping @parlough

parlough · 2023-09-19T18:31:56Z

Sorry again about the delay...

I don't want to land this until #9416 or a similar fix for dart and flutter being inaccessible in the container lands, but I think it may be better to just add the functionality to the code excerpt updater. It has all the information it needs, just needs a flag and to print them out.

AlexV525 · 2023-09-20T01:41:37Z

Sounds fair. I'll see if I can manage to make a request this week for the tool.

AlexV525 · 2023-09-23T04:02:00Z

Rethink about the diff, there is no proper way (for Dart officials) to diff changes between string content in Dart. The updater only updates files regardless of changes. So it might be better to use git diff as a solution.

parlough

Sorry about the delay @AlexV525!

You're right that using git diff will be much easier and provide better details.

One suggestion and a question, otherwise looks good to me.

parlough · 2023-10-22T21:44:26Z

tool/check-code.sh

 ) || (
+ git --no-pager diff


Won't this only show the diff if there is an error? I believe if all excerpts were successfully refreshed, it won't result an error, so the diff won't be shown.

Maybe this could be moved directly below the git reset? It's probably fine if it always runs.

Won't this only show the diff if there is an error? I believe if all excerpts were successfully refreshed, it won't result an error, so the diff won't be shown.

IIRC, from actual tests, refreshes will throw exceptions if there are codes been refreshed and return 0 if no codes have been refreshed.

If "successfully refreshed" means there are some changes during the refresh and the process ends properly, that is a misunderstanding.

I tried it out and the diff doesn't seem to run when excerpts were refreshed. I guess that's because executing the refresh script isn't the last executed command in the subshell.

I took another look of your suggestion, if all excerpts were refreshed, why the diff is required? The diff flow is only meant to help users to identify not refreshed files.

parlough · 2023-10-22T21:46:32Z

tool/check-code.sh

@@ -42,9 +42,14 @@ flutter --version
 if [[ $REFRESH ]]; then
 echo "=> Refreshing code excerpts..."
 (
+ commitHash=$(git reflog -n1 --format='%h')


Could you put this new behavior under a new flag, like --refresh-diff and then specify that in test.sh? So that those running this locally can opt in to the behavior.

Should we opt-in? IMO it should at least opt-out. This is helpful whether we test locally or remotely.

That's fine too. It would just be nice to have some way to disable it.

AlexV525 · 2023-10-26T06:08:06Z

Guys, I don't really like merging from the main branch too often. 😄 It'll somehow affect my acknowledgment of my branchs' status. It can be outdated, but without conflicts.

domesticmouse · 2023-10-26T09:35:34Z

Guys, I don't really like merging from the main branch too often. 😄 It'll somehow affect my acknowledgment of my branchs' status. It can be outdated, but without conflicts.

Keeping PRs up to date enables easier reviewing as the delta is more accurate

parlough

I think let's try this out then and see in what situations it helps. We can always expand when/where the diff functionality runs in the future.

Thanks for this Alex and again sorry about the wait.

Detecting code excerpts during checking codes would not tell what codes are refreshed during the check. This PR adds `git diff` result to tell the difference. This was inspired during our sync work and implemented in cfug/flutter.cn#1341. --------- Co-authored-by: Anthony Sansone <[email protected]> Co-authored-by: Parker Lougheed <[email protected]> Co-authored-by: Brett Morgan <[email protected]>

AlexV525 added 2 commits August 5, 2023 15:01

Update check-code.sh

e293717

Update check-code.sh

d34aebc

AlexV525 requested review from atsansone, parlough and khanhnwin as code owners August 5, 2023 07:38

Merge branch 'main' into feat/improve-refresh-code-excerpts

9e0e607

parlough self-assigned this Aug 9, 2023

Merge branch 'main' into feat/improve-refresh-code-excerpts

907bb8a

atsansone added the review.tech Awaiting Technical Review label Aug 26, 2023

Merge branch 'main' into feat/improve-refresh-code-excerpts

ae5dd5a

AlexV525 added a commit that referenced this pull request Sep 17, 2023

👷 Apply #9194

eac4f6e

AlexV525 and others added 3 commits September 17, 2023 14:08

Update check-code.sh

3072e2c

Merge branch 'main' into feat/improve-refresh-code-excerpts

1d6c5a7

Clarify commit message

2409e56

Merge branch 'main' into feat/improve-refresh-code-excerpts

e3f45ef

Merge branch 'main' into feat/improve-refresh-code-excerpts

380fb9b

parlough requested review from domesticmouse and MaryaBelanger as code owners October 22, 2023 21:30

parlough reviewed Oct 22, 2023

View reviewed changes

parlough assigned AlexV525 and unassigned parlough Oct 22, 2023

parlough added review.await-update Awaiting Updates after Edits and removed review.tech Awaiting Technical Review labels Oct 22, 2023

Merge branch 'main' into feat/improve-refresh-code-excerpts

4764e3e

Merge branch 'main' into feat/improve-refresh-code-excerpts

a157999

parlough removed the review.await-update Awaiting Updates after Edits label Oct 26, 2023

parlough approved these changes Oct 26, 2023

View reviewed changes

parlough merged commit e11bcec into main Oct 26, 2023
9 checks passed

parlough deleted the feat/improve-refresh-code-excerpts branch October 26, 2023 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve refresh code excerpts when checking codes #9194

Improve refresh code excerpts when checking codes #9194

AlexV525 commented Aug 5, 2023

github-actions bot commented Aug 9, 2023 •

edited

Loading

AlexV525 commented Aug 17, 2023 •

edited

Loading

parlough commented Aug 18, 2023 •

edited

Loading

AlexV525 commented Sep 17, 2023

parlough commented Sep 19, 2023 •

edited

Loading

AlexV525 commented Sep 20, 2023

AlexV525 commented Sep 23, 2023 •

edited

Loading

parlough left a comment

parlough Oct 22, 2023

AlexV525 Oct 23, 2023

parlough Oct 23, 2023 •

edited

Loading

AlexV525 Oct 26, 2023

parlough Oct 22, 2023

AlexV525 Oct 23, 2023

parlough Oct 23, 2023

AlexV525 commented Oct 26, 2023 •

edited

Loading

domesticmouse commented Oct 26, 2023

parlough left a comment

Improve refresh code excerpts when checking codes #9194

Improve refresh code excerpts when checking codes #9194

Conversation

AlexV525 commented Aug 5, 2023

Presubmit checklist

github-actions bot commented Aug 9, 2023 • edited Loading

AlexV525 commented Aug 17, 2023 • edited Loading

parlough commented Aug 18, 2023 • edited Loading

AlexV525 commented Sep 17, 2023

parlough commented Sep 19, 2023 • edited Loading

AlexV525 commented Sep 20, 2023

AlexV525 commented Sep 23, 2023 • edited Loading

parlough left a comment

Choose a reason for hiding this comment

parlough Oct 22, 2023

Choose a reason for hiding this comment

AlexV525 Oct 23, 2023

Choose a reason for hiding this comment

parlough Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

AlexV525 Oct 26, 2023

Choose a reason for hiding this comment

parlough Oct 22, 2023

Choose a reason for hiding this comment

AlexV525 Oct 23, 2023

Choose a reason for hiding this comment

parlough Oct 23, 2023

Choose a reason for hiding this comment

AlexV525 commented Oct 26, 2023 • edited Loading

domesticmouse commented Oct 26, 2023

parlough left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 9, 2023 •

edited

Loading

AlexV525 commented Aug 17, 2023 •

edited

Loading

parlough commented Aug 18, 2023 •

edited

Loading

parlough commented Sep 19, 2023 •

edited

Loading

AlexV525 commented Sep 23, 2023 •

edited

Loading

parlough Oct 23, 2023 •

edited

Loading

AlexV525 commented Oct 26, 2023 •

edited

Loading