Suggest the required form #235

jablko · 2020-11-24T16:07:19Z

Putting this out there for feedback.

Iterate on #224. Show the differences from the required form as suggestions. Like #224, only shows freshly introduced differences, not preexisting diffs.

elibarzilay

Seems reasonable overall, but given that this also does some deeper changes, I'll wait until the new code gets merged (#234 and a following PR to change the stale timelines).
(Sorry that it took so long, and it might take more time yet since it's thanksgiving week, so it's hard to find people to review stuff...)

Also, IIUC, then this supersedes #224, right? It's more extreme in what it does, but I like the approach of having proper feedback rather than rely on the short comment lines in the welcome message. So if it does, then should #224 be closed?

Also, I didn't see any tests for this?

elibarzilay · 2020-11-24T21:29:46Z

src/pr-info.ts

+        // Suggest the different lines to the author
+        const suggestionLines = (
+            Object.keys(suggestion).length <= 1
+                ? prettier.format(JSON.stringify(suggestion), { tabWidth: 4, filepath: ".json" })


Is prettier really needed here, if all it does is just suggest a bit of json to use?

It's overkill for JSON but I used it because it's uncomplicated. It minimizes the diffs from the existing JSON that are suggested to the author.

I'm not following this. AFAICT, the above just formats the json value from scratch, so how is it minimizing diffs?
In addition to that, there's the length condition that makes it use a plain stringify for short objects, and prettier can't do that then it's even worse to use it for some cases and do its job manually in other cases.

The format is closer to the files that are currently in the DT repo.

I'll ask around whether people mind adding it... But see also the comment below about using it only for some cases, which I still find disturbing, especially if prettier is included.

I finally talked to @RyanCavanaugh about it, and he doesn't like adding prettier here too...

Okay, thanks for asking around. I've removed it.

src/comments.ts

jablko · 2020-11-26T17:16:24Z

Seems reasonable overall, but given that this also does some deeper changes, I'll wait until the new code gets merged (#234 and a following PR to change the stale timelines).
(Sorry that it took so long, and it might take more time yet since it's thanksgiving week, so it's hard to find people to review stuff...)

All good, thank you for the reviews!

Also, IIUC, then this supersedes #224, right? It's more extreme in what it does, but I like the approach of having proper feedback rather than rely on the short comment lines in the welcome message. So if it does, then should #224 be closed?

Great! Done ✔️

Also, I didn't see any tests for this?

Done. I added DefinitelyTyped/DefinitelyTyped#49639

elibarzilay

Many more comments now, since I looked at the code more thoroughly.

Also, it would be good to add another test that shows suggesting adding a line, or even better, both adding and removing.

elibarzilay · 2020-11-27T08:23:53Z

src/comments.ts

+
+export const Suggestions = (user: string) => ({
+    tag: "suggestions",
+    status: `@${user} I noticed these differences from the required form. If you can revise your changes to avoid them, so much the better! Otherwise please reply with explanations why they're needed (unless it's obvious) and a maintainer will take a look. Thanks!`,


You probably have a good idea for how often exceptions are needed. If it's not common, then I think that the below is more effective in discouraging arguments.

Suggested change

status: `@${user} I noticed these differences from the required form. If you can revise your changes to avoid them, so much the better! Otherwise please reply with explanations why they're needed (unless it's obvious) and a maintainer will take a look. Thanks!`,

status: `@${user} I noticed these differences from the required form. If you can revise your changes to avoid them, so much the better! If they are absolutely needed, please reply with explanations why they're needed and a maintainer will take a look. Thanks!`,

Because this applies to tsconfig.json and package.json, as well as tslint.json, I'd like to see the current language in action and how people react to it first, then revisit based on the observed shortcomings.

OK, so maybe just drop the (unless it's obvious) comment? If it's really obvious, people will most likely not bother with it anyway.

Done ✔️

src/compute-pr-actions.ts

src/execute-pr-actions.ts

elibarzilay · 2020-11-27T08:35:52Z

src/execute-pr-actions.ts

@@ -106,6 +111,32 @@ function getMutationsForCommentRemovals(actions: Actions, botComments: ParsedCom
    });
 }

+function getMutationsForSuggestions(actions: Actions, pr: PR_repository_pullRequest) {
+    if (pr.reviews?.nodes?.find(review => review?.author?.login === "typescript-bot")) return [];


sameUser("typescript-bot", review?.author?.login || "-")

Even better, make it into a new isBot function in util/util.ts, and use it also in pr-info.ts..

IIUC, this is a once-per-PR thing? Shouldn't it be once-per-head, so it will re-post reviews after people edit stuff? (I'm thinking about someone seeing the suggestion, fixes one thing but leaves another, then dismiss the whole thing, and then a maintainer who is used to seeing these bot-reviews misses the whole thing.)

Done ✔️

Maybe I'm missing something, or maybe you didn't push more commits yet, but the code that I see doesn't do that.

But more than that, the code that I see posts a new review regardless of whether one exists: will the GH api update an existing one? If not, then it will post a new review on every run.

It excludes existing suggestions' diffs from subsequent suggestions, so suggestions will be [] if they were already posted. I should've added some comments, adding them now ✔️

src/pr-info.ts

src/execute-pr-actions.ts

src/pr-info.ts

elibarzilay · 2020-11-27T10:00:47Z

src/pr-info.ts

+        // Suggest the different lines to the author
+        const suggestionLines = (
+            Object.keys(suggestion).length <= 1
+                ? prettier.format(JSON.stringify(suggestion), { tabWidth: 4, filepath: ".json" })


I'm not following this. AFAICT, the above just formats the json value from scratch, so how is it minimizing diffs?
In addition to that, there's the length condition that makes it use a plain stringify for short objects, and prettier can't do that then it's even worse to use it for some cases and do its job manually in other cases.

elibarzilay · 2020-11-27T10:27:06Z

src/pr-info.ts

+        const lines = contents.split(/^/m);
+        let i = 0;
+        while (suggestionLines[i].trim() === lines[i].trim()) i++;
+        let j = 1;
+        while (suggestionLines[suggestionLines.length - j].trim() === lines[lines.length - j].trim()) j++;


Something's fishy here. In the example that you added, there is one difference of dropping a new await-promise key, so the resulting suggestion has zero length. It looks like GH will properly take it to show a suggestion to remove the lines but I'm not sure about it. If it will always do that, then there should be a comment saying that this could happen and that's as intended.

Also, I'm worried about this code depending on a difference, otherwise it would find a bogus i/j.

Also, this whole code is very verbose for something to include in a small function, so I'd rather see it move out into some makeJsonSuggestion thing that will do all of the necessary work.

Also, I think that the code itself could be improved. (Something along the lines of popping lines from both while they're equal, then finding the first index where they're different.)

Done ✔️

Looks much more readable -- one slight comment (feel free to ignore and I'll probably do it later: I find it even easier to read if all the variables are defined on one line:

const lines = newText.split(/^/m), startLine = 1, endLine = lines.length;

(because this way I don't need to look into the code and figure out that lines doesn't change).

No objection but I'm not sure what's best here in particular, because startLine and endLine aren't constant?

src/pr-info.ts

elibarzilay · 2020-11-30T22:56:04Z

src/compute-pr-actions.ts

+    context.suggestions = noNulls(flatten(info.pkgInfo.map(pkg => pkg.files.map(({ path, suggestion }) =>
+        suggestion && { path, ...suggestion }))));


This uses noNulls, but the list might have undefineds. I see that the function uses != instead of !== so it works, but it looks bad to rely on this. (I remember other places where I wanted to remove undefineds, and missed that it's doing it.) So it would be better to:

Rename it to something that indicates noNullsOrUndefineds but with some name that isn't so obnoxiously long. Or maybe just add a comment saying it?

Or make this code suggestion ? {...} : null?

elibarzilay · 2020-11-30T23:04:31Z

Looks closer overall. Two things in addition to the above:

Can you add a test that adds (hopefully also removes) lines? It's fine to just make a copy of the test you added and hand-edit it.
There's a complaint now about PR's ending up in the "Other" column when the PRs don't have the expected header -- instead of making it go to "Needs Author Action" with a comment saying what's wrong. It's not really related to this PR, but if you see a quick way to add it, it would be better than me making a separate PR which will probably have some bad conflicts with this one. If you don't get to it, I might do so as commits on top of this PR.

#235 (comment) Rename to `noNullish()`. I went with "nullish" like ["nullish coalescing"](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#nullish-coalescing), "nullish checks", etc.

jablko · 2020-12-03T16:39:15Z

Can you add a test that adds (hopefully also removes) lines? It's fine to just make a copy of the test you added and hand-edit it.

Done: 4183e39

I hand-edited another file in the existing fixture. The bot adds the line that I hand-removed.

orta

Not read the code too deeply other than tests which seems legit, but this is a clever idea 👍🏻

Co-authored-by: Eli Barzilay <[email protected]>

elibarzilay reviewed Nov 24, 2020

View reviewed changes

jablko mentioned this pull request Nov 25, 2020

Show differences from the required form #224

Closed

jablko force-pushed the patch-15 branch 3 times, most recently from f9c7057 to 87a49f7 Compare November 26, 2020 16:50

elibarzilay suggested changes Nov 27, 2020

View reviewed changes

jablko force-pushed the patch-15 branch from 7de8392 to cd8ebd7 Compare November 30, 2020 18:10

elibarzilay reviewed Nov 30, 2020

View reviewed changes

jablko mentioned this pull request Dec 2, 2020

Rename noNulls() -> noNullish() #249

Merged

jablko force-pushed the patch-15 branch from cd8ebd7 to 72c326f Compare December 3, 2020 16:23

jablko force-pushed the patch-15 branch 5 times, most recently from 7c8adaf to 3c981c3 Compare December 8, 2020 21:21

jablko mentioned this pull request Dec 9, 2020

Put explanations next to suggestions #265

Draft

jablko force-pushed the patch-15 branch 4 times, most recently from d823ee3 to 4af0c0e Compare December 12, 2020 18:56

orta approved these changes Dec 15, 2020

View reviewed changes

jablko force-pushed the patch-15 branch 4 times, most recently from c764954 to 7176fba Compare December 20, 2020 16:44

jablko force-pushed the patch-15 branch from 7176fba to 4fea6f7 Compare December 22, 2020 14:50

jablko force-pushed the patch-15 branch 5 times, most recently from d467d32 to 8a76ccd Compare January 20, 2021 22:58

jablko and others added 17 commits January 31, 2021 14:26

Show differences from the expected form

3963a63

Update snapshots

422f1c2

Apply suggestions from code review

cca75d6

Update snapshots

3040156

Suggest the expected form

0aa7fa3

Update snapshots

a9430ed

Add tests

c3567a3

Apply suggestions from code review

a9faadd

Co-authored-by: Eli Barzilay <[email protected]>

Update snapshots

a554fde

Add tests

0387d07

Cosmetic

1211f3c

Extra types

ee4c74a

Fix line endings regex

1542d2d

Update snapshots

47f96e8

Apply suggestions from code review

3602444

Update snapshots

ed94b1c

Remove Prettier

3c0650d

jablko force-pushed the patch-15 branch from 8a76ccd to 3c0650d Compare January 31, 2021 21:28

elibarzilay force-pushed the master branch 3 times, most recently from 1d39a68 to 9e931d1 Compare July 7, 2021 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest the required form #235

Suggest the required form #235

jablko commented Nov 24, 2020

elibarzilay left a comment •

edited

Loading

elibarzilay Nov 24, 2020

jablko Nov 26, 2020

elibarzilay Nov 27, 2020

jablko Nov 30, 2020

elibarzilay Nov 30, 2020

elibarzilay Dec 11, 2020

jablko Dec 12, 2020

jablko commented Nov 26, 2020

elibarzilay left a comment

elibarzilay Nov 27, 2020

jablko Nov 30, 2020

elibarzilay Nov 30, 2020

jablko Dec 3, 2020

elibarzilay Nov 27, 2020

jablko Nov 30, 2020

elibarzilay Nov 30, 2020

jablko Dec 3, 2020

elibarzilay Nov 27, 2020

elibarzilay Nov 27, 2020

jablko Nov 30, 2020

elibarzilay Nov 30, 2020

jablko Dec 3, 2020

elibarzilay Nov 30, 2020

jablko Dec 3, 2020

elibarzilay commented Nov 30, 2020

jablko commented Dec 3, 2020

orta left a comment

	status: `@${user} I noticed these differences from the required form. If you can revise your changes to avoid them, so much the better! Otherwise please reply with explanations why they're needed (unless it's obvious) and a maintainer will take a look. Thanks!`,
	status: `@${user} I noticed these differences from the required form. If you can revise your changes to avoid them, so much the better! If they are absolutely needed, please reply with explanations why they're needed and a maintainer will take a look. Thanks!`,

		context.suggestions = noNulls(flatten(info.pkgInfo.map(pkg => pkg.files.map(({ path, suggestion }) =>
		suggestion && { path, ...suggestion }))));

Suggest the required form #235

Are you sure you want to change the base?

Suggest the required form #235

Conversation

jablko commented Nov 24, 2020

elibarzilay left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jablko commented Nov 26, 2020

elibarzilay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elibarzilay commented Nov 30, 2020

jablko commented Dec 3, 2020

orta left a comment

Choose a reason for hiding this comment

elibarzilay left a comment •

edited

Loading