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

Ignore "Silence is golden" files #99

Open
davidsword opened this issue May 27, 2020 · 5 comments
Open

Ignore "Silence is golden" files #99

davidsword opened this issue May 27, 2020 · 5 comments

Comments

@davidsword
Copy link

If a index.php file contains just the standard "//Silence is golden" comment, the bot reports that hashes-to-hases approves the file, which should go without saying.

Screen Shot 2020-05-27 at 3 36 39 PM

Might help reduce bot noise by exempting this file with that content.

@GaryJones
Copy link
Contributor

The Hashes integration only looks at the hash of the file content, which is the sha1 once whitespace and comments has been stripped.

That means that if they had added a full stop to the comment, as per WPCS, or a space after the //, or a clear linespace at the end, or put the comment on a new line (all valid variations), then it should be the same hash.

The userscript we have (private repo, so won't link to it) for reviews, specifically looks for this Hashes comment though, and automatically marks the file as Viewed and closes it when reviewing it. So the presence of the comment has benefit for us.

I get the point about the noise. For clients, a comment on an empty file like this doesn't add anything, but then, it doesn't for any file it appears on; unless that is, someone is using the Review Bot comments to see what VIP has already marked as good, and so does find value in that comment being there, even if it's clearly for a file that doesn't have any active code in it. As such, adding exclusions for this may cause confusion for no reason.

There’s nothing special, or standard about "Silence" files like these - it’s not even a good practice, since it should be configured at the server level to avoid directory listing, and having it named as index.php won’t work if the DirectoryIndex or equivalent setting is set to something else anyway.

Rather than adding exclusion for a Review Bot comment for this particular hash, I'd rather see us / the Review Bot encourage that these empty files are removed completely, since they serve no purpose on VIP, and are not protecting anything on a local set up.

@gudmdharalds
Copy link
Contributor

Hi!

Thanks for opening this issue.

Yes, I can understand how these comments can be a bit annoying. However, they are not specific to these empty files, as Gary noted above. These comments are also used by the userscript.

I've thought a bit about if there any other ways for vip-go-ci of conveying that a particular file is approved in the hashes database, but given GitHub's features, I think this is the most practical way, though it can be a bit noisy.

Is there any other less noisy ways of doing this? I don't see any. I'm happy to look into options.

@GaryJones
Copy link
Contributor

@gudmdharalds Does the GH API allow for marking a file as Viewed for all viewers?

@gudmdharalds
Copy link
Contributor

Does the GH API allow for marking a file as Viewed for all viewers?

Not to my knowledge. I looked into the API call done by the browser when one presses Viewed in Pull-Request and it seems to be tied to the current user. I haven't found this feature in the documentation.

@gudmdharalds
Copy link
Contributor

It would be possible to add exception for empty files, so that these comments are not posted for them.

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

No branches or pull requests

3 participants