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

Automated code review for metal-rs #133

Open
gingerDevilish opened this issue Apr 6, 2020 · 18 comments
Open

Automated code review for metal-rs #133

gingerDevilish opened this issue Apr 6, 2020 · 18 comments

Comments

@gingerDevilish
Copy link

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
Copy link
Member

kvark commented Apr 8, 2020

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

@kvark
Copy link
Member

kvark commented Apr 8, 2020

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"?

@gingerDevilish
Copy link
Author

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.

@gingerDevilish
Copy link
Author

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
Copy link
Member

kvark commented Apr 10, 2020

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
Copy link
Member

kvark commented Apr 10, 2020

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...

@gingerDevilish
Copy link
Author

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
Copy link
Member

kvark commented Apr 10, 2020

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

@gingerDevilish
Copy link
Author

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.

@gingerDevilish
Copy link
Author

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
Copy link
Member

kvark commented May 4, 2020

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

@gingerDevilish
Copy link
Author

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
Copy link
Member

kvark commented May 5, 2020

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

@gingerDevilish
Copy link
Author

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
Copy link
Member

kvark commented May 25, 2020

@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! :)

@gingerDevilish
Copy link
Author

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:

  • Clippy support!
  • Then, we're going to focus on adding more checkers

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

@gingerDevilish
Copy link
Author

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!

@gingerDevilish
Copy link
Author

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!

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

2 participants