Remove noisy documentation preview comment #37387
Conversation
|
-1 on the removal. People who do not want to see it can trivially filter it out in their email system. |
I don't get any emails from github. I and other are annoyed by the notifications in the github interface, which you cannot filter. Moreover, the functionality to preview the docs is still there of course, you just have to click in a different place: |
tobiasdiez
added
p: critical / 2
disputed
|
Documentation preview for this PR (built with commit 7fdb169; changes) is ready! 🎉 |
Of course it's still there. The valuable point of the notification is ... to be notified, the importance of which is explained in detail in #37387 (comment) |
|
Could we drop the notification but change the PR checklist to tell people to check the generated documentation? (So, less noise but developers are still encouraged to make sure the documentation works out?) |
|
In the case there might be a vote at some point: I am strongly in favour of removing it. I check the doc before I give Positive review, but the bot notifies me after every push, and inconveniently a few hours later. |
It's actually curious. Other integrations of bots that just update an existing comment manage to not trigger a notification, e.g. this one flatsurf/flatsurf#336 (comment) (code coverage.) The first post, notifies you, the following updates are silent. In any case, I would just add instructions to the checklist where to find the documentation. The posts by the bot here contain no information I think, other than the link to the documentation that can be found elsewhere. (That's different in the case of the coverage bot linked above. That bot alerts you that the coverage dropped for example.) I am happy to update the PR if that seems reasonable to you @mkoeppe. |
|
-1 (strong if you ask) I use the links in the comment while reviewing any PR. The doc-preview comment also contains a link to "changes", which could be useful (yes, not always). I am actually surprised that some people find the doc-preview comment on github interface annoying. Why is it noisy? I don't receive emails for it. It's just one comment of a single line. |
|
It's noisy because every time someone updates their branch with the newest beta or release candidate I get a blue dot. I then think I have to do something (re-review, or check a comment on my own pr), but there is nothing. |
@kwankyu would that work for you? |
|
@saraedum Our review instructions already say very clearly that the built doc needs to be inspected. As I said above in #37387 (comment), the point is that this step has been skipped too many times in the past. I understand that some may find the Github notifications noisy, but to me this seems a very minor DX concern compared to the project's concern to have quality documentation for its users. And for me as a developer with a high volume of PRs, being able to work in a notification-driven way (not waiting for a local docbuild to finish and not having to take manual note of the task to check the built doc later) is crucial for productivity. |
|
What should be investigated instead:
|
|
@kwankyu Another thought: Would we be able to download a previously deployed documentation archive from netlify? And then suppress the preview comment if there was no change compared to that? |
Why would people skip that step if it's in the checklist? If people skipped the checklist, there's nothing to review to me. |
Not sure to what depth you're hoping for me to explain human behavior... My point is that they do. |
|
Why would they not skip the comment from the bot? Do you have any reason to believe that that one is more effective than a checklist item? (rephrased.) |
Isn't there an option to get notified by email? |
Actually, there is https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/about-notifications#subscription-options so apparently you can get notified on runs that you triggered. (Though I fail to understand how to enable this.) |
Reminders work, in particular if they include the specific thing that needs checking (the diff links) |
|
FWIW, I'm getting the notifications by email. and the emails are what prompts me to check the built docs. |
For me, they do not work. Personally, I find it really annoying if people "remind" me about things that they asked me to do less than a week ago. In this particular case, these reminders just mean that I ignore all sagemath/sage email from GitHub (though I could fix this with a filter) and also that I cannot navigate my inbox for sagemath here on GitHub at times. |
| with: | ||
| number: ${{ steps.source-run-info.outputs.pullRequestNumber }} | ||
| header: preview-comment | ||
| recreate: true |
There was a problem hiding this comment.
So, if we just drop this line. Then the noise is gone as well and we get a single comment that updates (silently) instead?
There was a problem hiding this comment.
No. Why do you ask the same question again? It has been clearly answered. The notification is what people who are doing the work want to get.
There was a problem hiding this comment.
@mkoeppe , actually no. The notification gets on my nerves. But more importantly, it may lead to me not noticing important messages, because I click "mark all as read"
There was a problem hiding this comment.
Why do you ask the same question again?
I do by no means see this as a repeated question.
And I am trying to find some middle ground here between those who are annoyed by these notifications and those who want to keep them. With the proposed change, you still get all the relevant links. But if you want push notifications, you'd have to get these by other means.
The notification is what people who are doing the work want to get.
I hope you are not trying to imply anything with "people who are doing the work". There seems to be a substantial amount of people here who do not want these notifications.
So, unless I misunderstood you here. You want a notification and email every time the workflow finishes. So you want, what others consider spam. That's your opinion and you are entitled to it. But since that's precisely what others do not want, we have apparently no hope to come to a compromise here.
Previously, the documentation comment on GitHub would be removed and then recreated which creates notification emails and to some feels like it's polluting their GitHub inbox. Here, we change this behavior so that only the initial build of the documentation causes such a notification. Further updates to the documentation, do update the links in the comment but do not trigger a notification. This is meant as an alternative to sagemath#37387. See sagemath#37739 for futher ideas about a smarter behavior here.
|
I created #37740 as a solution that tries to find some middle ground as outlined above. |
The PR description has two 👍 (not counting the author) and one 👎 if I'm not mistaken. But that was not my point.
I thought this was addressed here: |
Apparently I need to be more direct. In #37387 (comment), #37387 (comment), now #37739, we have sketched a clear approach that would reduce the notifications in a way that would not diminish the value of this productivity tool. This should be given proper consideration; so far there has not been any response. |
True. As an alternative, we could just consider to update the PR template to tell people where to find this. If we don't tell people, I don't think anybody would click there :) |
I don't see how this is immediately related to the problem that we are trying to solve here. (Sure, it makes one of the links somewhat less useful but this PR is about the notifications that people get.)
That's an alternative approach but it's substantially more work to implement. And I'd rather not receive notifications at all to be honest. |
That doesn't rise to the standard "proper consideration". |
Could you please explain "what" does not? And also which standard of "proper consideration" are you referring to? |
|
@saraedum In #37387 (comment) I wrote that the solution sketched in #37739 "should be given proper consideration". Saying that it's "substantially more work to implement" and mentioning your previously stated personal preference regarding the notifications is not "proper consideration". I'm of course aware that it's more work to implement; that's why I've marked it as a "wishlist item". I would suggest that those who feel bothered by the notifications invest an appropriate amount of effort in it. |
I disagree. I consider that I have given it proper consideration.
I also don't agree here. Why should those who would like to completely get rid of the notification invest into making it better? That's why I propose to not send further notifications after the first one. It's a middle ground. (That it's easy to implement is another bonus of course.) Let's not get caught up on the technicalities of who should be doing what supposedly. (Just worried that we're going off topic here. I am happy to chat about this on Zulip or elsewhere.) |
Because that's how consideration and collaboration should work in our community. The purpose and value of the documentation preview notification feature have been explained clearly and thoroughly. I have seen nothing brought forward here that would explain why piling up notifications should be considered more than a minor developer experience concern, and in particular why that should weigh more than what the feature offers.
The standard warning about the fallacy of the golden mean applies. This proposal is not suitable as a compromise -- because it completely ignores the use of this notification as a workflow management tool. #37387 (comment) |
I don't see it that way. And I don't think it's realistic that people will want to spend their free time like that.
Indeed. But I think it's also been made clear why several people find this feature very annoying.
I don't think it's considered "minor" by those who don't like it. Certainly, I consider it major to the workflow you describe but obviously that's colored by me not using that workflow. Also, we have seen several people say that they are annoyed by this feature but so far only you have said that you use it for your workflow. (Am I missing somebody?)
I am sorry, but from my perspective neither is your insistence on keeping the notifications essentially as they are (I understand that you see #37739 as a compromise of sorts but I don't see this as making a change to the actual problem here.) Again, I don't think this ticket is the right place to discuss who should work on what and who should decide on what. |
Julian, kind reminder that this word has no place in a discussion.
How can sending fewer notifications (per #37739) not be relevant as a possible compromise? |
|
Another possible variant: Have only the PR author receive the docbuild notification.
|
This comment was marked as resolved.
This comment was marked as resolved.
It would make more sense to me if the reviewers are notified. |
|
@mantepse Let me try to explain the rationale for my suggestion better: In the notification-based workflow, a developer is working on multiple PRs simultaneously. After pushing a change to one PR, they can immediately switch to working on a different PR. When notifications come in (either "CI failed" or "documentation ready for inspection"), they can act on the notifications -- either by fixing the failure or by checking the documentation. In this way, the notifications enable a work schedule that is free of waiting time. |
|
Ok. Well, I usually work on a single topic, and my efficiency is limited not by waiting time but rather my brain and interest. So, this workflow is not for me. |
|
This workflow also works as well for developing several related PRs on the same topic. |
|
Closing this in favor of the compromise #37740. |
|
I'm removing the "disputed" tag since this was closed by the author. If the PR gets reopened, please add the tag again. |
jhpalmieri
removed
the
disputed
Previously, the documentation comment on GitHub would be removed and then recreated whenever there is a change to a PR which creates notification emails and to some feels like it's polluting their GitHub inbox. Here, we change this behavior so that only the initial build of the documentation causes such a notification. Further updates to the documentation do update the links in the comment but do not trigger a notification. This is meant as an alternative to sagemath#37387. See sagemath#37739 for futher ideas about a smarter behavior here. An actual preview of the result can be seen here: saraedum#2 Note that the links on that PR do not work because my fork does not have netlify credentials configured. ### 📝 Checklist - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes; this is fairly hard to test automatically. I'll test this manually if there is interest in merging this. - [x] I have updated the documentation accordingly; kind of, I updated the PR checklist. URL: sagemath#37740 Reported by: Julian Rüth Reviewer(s): Dima Pasechnik, Julian Rüth, Kwankyu Lee
On behalf of the Code of Conduct Committee: we do not see any problem with the word "insistence". It does not have the same negative connotations as some perhaps similar words. It is also not clear what "reminder" refers to, since the word has not come up before this. |
The comment linking to the built docs is noisy and doesn't add much value: the docs can also easily be accessed by clicking on the "Successfully deploy preview" status check or via the summary of the deploy workflow.
On the other hand, it always triggers a notification. Hence, we remove the comment.
📝 Checklist
⌛ Dependencies