Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Always show the list of files to check #285

Merged
merged 1 commit into from
Dec 19, 2020
Merged

Always show the list of files to check #285

merged 1 commit into from
Dec 19, 2020

Conversation

elibarzilay
Copy link
Contributor

Motivated by the recent change of showing the Check Config label also
for new packages, where in such cases the usual place where the
suspicious configs are shown is not reached since there's an earlier
case that shows a different comment for new packages. Instead of
showing the list of files there too, this just moves the whole thing
into the packages display section (which shows the links and pre-package
approvals).

Motivated by the recent change of showing the `Check Config` label also
for new packages, where in such cases the usual place where the
suspicious configs are shown is not reached since there's an earlier
case that shows a different comment for new packages.  Instead of
showing the list of files there too, this just moves the whole thing
into the packages display section (which shows the links and pre-package
approvals).
@elibarzilay
Copy link
Contributor Author

For example, see below for how DefinitelyTyped/DefinitelyTyped#50159 is shown with this. Note also that this makes it a bit more visible, and I'm guessing that we'll need to improve the code as a result -- in this case, the complaint is because there's a "DOM" added to compilerOptions.lib.

image

@elibarzilay
Copy link
Contributor Author

@jablko: I didn't check but this is likely to conflict with the big PR for adding suggestions-as-reviews, but I think that it's a good idea to use this as a first step in adding it gradually:

  • Start with this, which shows the list in a consistent place so people will see it more.
  • Would be nice to then add a short one-line explanation for each bit that made the change suspicious (in the above case, the DOM addition).
  • This would make it even more useful (for both contributors and maintainers), and will probably lead to complaints about things like this dom thing where the bot should do a better job. (I think that this is a very good thing to do before the last step ➡️)
  • Add the suggestion reviews. (:arrow_right: because once it's a review, it should be very robust since many people will probably just blindly accept whatever the bot suggest, and will be annoyed if it's incorrect.)

Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

I agree with the ideas 👍🏻 - implementation looks good too

@elibarzilay elibarzilay merged commit f2cc960 into DefinitelyTyped:master Dec 19, 2020
@elibarzilay elibarzilay deleted the always-show-configs-to-check branch December 19, 2020 14:11
@jablko
Copy link
Contributor

jablko commented Dec 22, 2020

Can we put the explanation next to the line in question? I'll prepare a PR based on #265, minus the actual suggestions.

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

Successfully merging this pull request may close these issues.

3 participants