gfx-rs / metal-rs

Rust bindings for Metal
Apache License 2.0
567 stars 112 forks source link

Automated code review for metal-rs #133

Open ghost opened 4 years ago

ghost commented 4 years ago

Hi! I am a member of the team developing monocodus — a service that performs automatic code review of GitHub pull requests to help organizations ensure a high quality of code. We’ve developed some useful features to the moment, and now we’re looking for early users and feedback to find out what we should improve and which features the community needs the most.

We ran monocodus on a pre-created fork of your repo on GitHub https://github.com/monocodus-demonstrations/metal-rs/pulls, and it found 15 formatting suggestions according to rustfmt. I hope that this information will be useful to you and would be happy to receive any feedback here or on my email alena@monocodus.com

We really love the Rust community, and we would love to hear from you on what automated checkers you might want to have. If there is something you cannot do with clippy, or cargo-fix, something you might have seen as a tool for some other language ecosystem, we can step forward and create it if you have any idea what you need!

If you want to try our service, feel free to follow the link: https://www.monocodus.com The service is entirely free of charge for open source projects. Hope you’ ll like it :)

kvark commented 4 years ago

We'd be happy to try this out on some projects. Thank you for the heads up!

kvark commented 4 years ago

I'm surprised you need "We ask for read and write access" though. Isn't the goal to generate PR suggestions? How does this require "write"?

ghost commented 4 years ago

Could you please show me where you have seen that? I'm pretty sure we did not set the requested permissions for the write access, as we are currently stalled on the features that would require that, and discussing the further course of action. It must be a textual mistake on our side.

That said, the team has been discussing approaches to comments on formatting (it's clear that it is not especially feasible to point out such things on the line-to-line basis), and we have some feature requests for automated reformatting triggered by a respective command/mention, which would indeed require the write access, but it's not the case at the moment.

ghost commented 4 years ago

Okay, I re-checked it with the team, and here's what is actually happening. While we don't request write access to your code, we do request it to pull requests: telegram-cloud-photo-size-2-5224537466445344351-x

This is necessary in order to post review commens and thus required for the core bot functionality.

It still seems we have to reformulate that phrase so that there is more clarity.

kvark commented 4 years ago

This is what scares me off:

By signing up, you are agreeing to our Terms of Service and Privacy Policy. We ask for read and write access. Verified email on GitHub is required.

If that's truly about only PR writes, it's very important to be clear about this in one of the first sentences that the user sees.

kvark commented 4 years ago

I signed up now. So it looks like Monocodues comes with a number of services built in. The first thing I would want is being able to configure (on a per-repository basis) which services are enabled, and which are disabled. Otherwise, there is going to be a storm of suggestions we don't want yet. I'm not seeing this ability anywhere in the UI...

ghost commented 4 years ago

We are going to roll out the config files soon, which will hopefully allow to fine-tune and configure tools not just on the per-repo but even on the per-directory basis (we all know how there are often different measures of good practices in tests and elsewhere) — it's in the works, likely next week.

As for the UI configuration, that's going to be rolled out with the UI rework which depends on the front-end contractor so far. Would it be more convenient for you to put the config file(s) in the target repo, or to make adjustments via the UI?

kvark commented 4 years ago

Yes, putting configs in the repo is totally fine with us.

ghost commented 4 years ago

Hi! I'm coming with the news that we have rolled in the configuration files. You can consult the documentation here.

So far, we don't have many tools attached, so if you have an idea for what you might want or need, we'll be happy to hear. We are also considering moving towards using GitHub Checks functionality, at least in part.

Any feedback is highly appreciated.

ghost commented 4 years ago

Hey, I'm coming back for a quick check-in. Did you try to set up a config? If so, did any difficulties arrive?

kvark commented 4 years ago

It was fairly straightforward so far, we disabled rustfmt and JS passes on some of the projects. Thank you!

ghost commented 4 years ago

Okay, good to know 👍 If you happen to wish for a UX tweak, find a bug, or have a feature request, feel free to reach out!

kvark commented 4 years ago

I sent 2 emails to support on April 20th without any sign of an answer.

ghost commented 4 years ago

Hey, just a quick check-in again. How has it been lately?

We have improved some wordings a while back during past few weeks, and are currently working on the architecture revamp to allow more substantial checks to be added. This summer, we are considering the integration of some of these verification tools. Is there something in the list that would be of use to your team?

Also, here's our public issues tracker in case you'd like to throw in some suggestions.

kvark commented 4 years ago

@gingerDevilish thanks for checking in!

Monocodus was a forcing function for us to move the codebases to be rust-formatted. It was a concern earlier that if we do this, it would be hard to keep it formatted: either we'd land non-formatted changes and try to clean them up, or we fail on CI, which is annoying to users. Monocodus strikes me as a great alternative - it instantly suggests changes, basically resolving the concern for us.

Is there something in the list that would be of use to your team?

We haven't used anything in this list, I think, but overall we'll appreciate anything in this direction.

The very basic tool that a lot of project use is clippy. We use it too, and we'd love to see it integrated with Monocodus. It would need to be configurable though: one of the simple use cases is only blocking on Clippy errors, in which case we wouldn't want to spam the warnings with Monocodus.

Also, here's our public issues tracker in case you'd like to throw in some suggestions.

Great, looking forward to complain a lot! :)

ghost commented 4 years ago

Hi @kvark!

I hope you're doing well.

We rolled an update just today, and here's what we got now:

  1. Support of GH's multiline suggestions — now suggestions from our autofix tools should become much less confusing. Besides, as we have reworked the algorithm of file processing, more fixes are suggested and fewer dumb missuggestions will occur.
  2. We have temporarily disabled some of the checkers for C, Python, and JS, because of them being deemed too spammy. They'll return later after a major rework, removing annoying notifications because of just one extra line above a threshold. We're seeking to only point out real prospective problems, not to be meddley.

Coming next:

Let us know if you have any issues — and stay tuned!

ghost commented 4 years ago

Hi!

I hope you're doing fine.

We have just deployed an update — this time this is clippy support. To enable it, you need to update your config to version 1.1.0, plus include native clippy configs in your projects if you need more fine-tuning.

So far we've got a spammy mode where we output all the comments we can place in the diff, and a succinct mode which only tells you that issues were found, so you may want to run the tool on your computer and fix them. Please let us know if you need any additional configuration options with respect to the monocodus output!

We're also looking for the best way to convert the clippy advice to automated suggestions while retaining explanations, so you can expect more automation soon.

Happy coding, and have a great summer!

ghost commented 4 years ago

Hi!

I'm happy to announce that we've just released suggestions integration for clippy — so now you can apply the fixes in a couple of clicks :) You don't need to do anything this time — no configuration changes are required.

Please let us know if you see any issues, or if you have any other requests — and tell your friends about us if you like the ways monocodus is trying to help you 😸

Happy coding, and stay tuned!