💾 Archived View for dioskouroi.xyz › thread › 25007046 captured on 2020-11-07 at 00:55:08. Gemini links have been rewritten to link to archived content

View Raw

More Information

-=-=-=-=-=-=-

Show HN: A Django code review bot for GitHub pull requests

Author: rikatee

Score: 98

Comments: 58

Date: 2020-11-06 12:11:11

Web Link

________________________________________________________________________________

wfleming wrote at 2020-11-06 13:05:30:

For anyone who likes the idea but doesn't use Django, check out

https://restyled.io/

- it supports a bunch of tools [1].

It's also open source and self-hostable for folks who like the idea but don't trust that automation to a third party.

[1]:

https://docs.restyled.io/available-restylers/

marcinzm wrote at 2020-11-06 13:20:49:

Sending all your code to a free third party tools seems risky to me. Any entity without a currently working business model but your data (or access to your systems) is going to be very tempted to abuse that access for money. Maybe not now but down the line after it gets sold to a sketchy Russian company (like a lot of browser extensions).

wfleming wrote at 2020-11-06 14:01:06:

I don't think this critique applies to restyled - it's free for use on open source repos, but costs money for private repos.

Which is a pretty standard arrangement for SaaS dev tools that I think makes it pretty clear your data isn't the business model. What I like about that model it is a reasonable balance of customers actually paying for the thing they use (so the business owner isn't beholden to advertising or tempted to abuse data access), and supporting the open source community (without which most of the modern web couldn't exist). I won't say it's entirely without risk - you need enough paid use to cover the costs of the open source use. But I think it's an ethical and sustainable approach if you can achieve that.

And, again, restyled is open source - if you don't trust the owner, you can run it yourself.

marcinzm wrote at 2020-11-06 14:35:37:

I agree, my comment was actually as to why you'd want to use something like Restyled over Django Doctor, sorry about not being clearer.

rikatee wrote at 2020-11-06 14:39:29:

Not to nitpick, but restyled and Django Doctor do different things in different ways, so I don't think it's and OR

rikatee wrote at 2020-11-06 14:37:46:

Our business model will soon become clear: oranisations will pay. Individuals not. Currently still ironing out things and seeking feedback so at the moment free to all so I can get maximum feedback :)

btw, we dont actually have any of the user's data. We literally do not have a user database. All we have is a link to a public repo. It currently works only on public repos. So no danger of Django Doctor doing anything odd with your code because the code is already public. No risk of Django Doctor doing anything strange with your personal data because we don't have any :)

sologoub wrote at 2020-11-06 17:49:13:

If I was still actively working on Django projects, probably would be willing to pay. Value seems fairly clear and proof of value would be straightforward to evaluate.

When I worked on solo projects, sanity checks like these would be very helpful since you don’t have a teammate to help spot “brain fart” situations.

rikatee wrote at 2020-11-06 18:05:58:

Thanks! Even the simple "Syntax Error" comment could be the difference between bed time and another cup of coffee.

richardARPANET wrote at 2020-11-06 13:24:32:

Walking across the street is also risky.

marcinzm wrote at 2020-11-06 13:26:56:

This is more like walking across the street blindfolded for no good reason. You can run similar tools locally or hosted yourself or open source.

edit: Or even a paid alternative as those at least seem to have a business model.

rikatee wrote at 2020-11-06 14:54:27:

in the case of Django Doctor - there is no direct alternative you can run locally. Nothing else does this:

- finds Django anti patterns. pretty sweet.

- suggests the fix in the PR. One click fix? yes pls.

- Check your entire codebase in seconds via

https://django.doctor

with no need to install or sign up for anything? hell yeah.

- has a fox as a mascot

rat9988 wrote at 2020-11-07 00:56:23:

For some odd reason the last point rubbed me the wrong way. It must remind me of some bland marketing / fans discussion.

richardARPANET wrote at 2020-11-06 14:01:57:

If you don't value your time, sure.

cosmic_quanta wrote at 2020-11-06 13:53:48:

I just want to say that I studied the restyled.io source code (

https://github.com/restyled-io/restyled.io

) when learning how to build a Haskell web app. Great stuff!

rikatee wrote at 2020-11-06 15:07:14:

Nice! They're quite different beasts though. Restyled enforces code style, while Django Doctor suggests fixes for Django specific anti patterns and has a fox for a mascot. See. Completely different ;)

iruoy wrote at 2020-11-06 14:16:50:

These are all standard tools. Why wouldn't you just run them as part of your CI workflow?

rzodkiew wrote at 2020-11-06 13:00:54:

I'm probably old fashioned, but I don't understand why would I want something like that as a web-service, rather than something I can run locally (like all the js linters or rubocop).

cameronh90 wrote at 2020-11-06 13:24:27:

It's not either/or. We use them in case a developer forgets to run them, or hasn't set their environment up properly or similar.

raziel2p wrote at 2020-11-06 13:51:52:

That's why you make it part of the CI pipeline and require it to pass before merging, making it impossible to forget.

The only reason you'd release a tool like this as a github integration and not an open source library (optionally with a CLI tool) is that you want to get money from it - nothing wrong with that, but that is very much an either/or question.

rikatee wrote at 2020-11-06 14:34:45:

Blocking the build works well for things that are boolean "correct" or "not correct", but some things are not are just "food for thought". Django doctor points out when something looks like it can be improved, but that's ultimately up to the dev to take the advice. It's "have you thought of..." rather than "you must do this".

With the advantage that if the dev likes the advice, all they need to do is click "commit"

rikatee wrote at 2020-11-06 14:31:53:

I've been in a few organization with hundreds of repos. It can be a pain to upgrade libraries. In this context, with SaaS you don't need to.

But yes, SaaS is not a silver bullet. If you like convenience and getting updates, bug fixes, and quick set up with no overhead then it's for you. If you want to run things yourself then understandable have a great day :)

nickjj wrote at 2020-11-06 15:05:26:

> It can be a pain to upgrade libraries. In this context, with SaaS you don't need to.

I see manually updating libraries as a feature that allows for deterministic linting.

I wouldn't want my linter to always be updated to the latest version because that might introduce warnings or deprecation errors that aren't warnings or errors in the version of the tools used in the code I'm linting.

rikatee wrote at 2020-11-06 15:10:15:

Agreed in general, but Django Doctor is not a linter. It suggests code improvements the dev can choose to commit with one click. Linters block builds until the code conforms to a pre set style, often requiring the dev to manually do it themselves. Django Doctor gives "food for thought" that the dev can follow or ignore.

aequitas wrote at 2020-11-06 15:26:30:

The same can be said for a linter. You can just ignore the lint warning and have it not block the build as well.

Most linters also don't require manual changes but do automatic fixing, ie: black[0].

Non-user initiated updates to QA tools will only lead to alert fatigue because people will write it off as an update to to tool.

[0]

https://github.com/psf/black

rikatee wrote at 2020-11-06 15:33:48:

Maybe I'm grandiose but I see the "form factor" as key: MP3 players existed before ipods. Some people found the new form factor more convenient.

Similarly they way Django Doctor works is more convenient:

- it's suggests a change that can be applied in one click

- it's suggestion can be ignored

- no overhead cost because it's SaaS

but yes, if you don't value SaaS in this context then that's fair enough :)

aequitas wrote at 2020-11-06 16:59:29:

It might be because I'm old fashioned and come from a terminal background (you kids with your MP3's, I'll stick with tapes). But for me the SaaS is the overhead. Moving away from my terminal and local dev environment. Waiting for web requests to return before I can continue really breaks my workflow. Also the idea that formatting and other coding standards are not decleratively enforced (and autofixed) but an arbitraty choice during review has become strange to me over the past few years. As with Black for example. Every choice about formatting can be premade and automatically applied by a computer. This saves so much time in the review process. No bike-shedding about tabs or spaces. Just conform to one autoformatted standard and optionally convert it to your local preference (and back) in your IDE/Git-hooks.

That said, if you live in Github and other SaaS services this fits really well. In the past I've setup similar workflow to cater to people who never dared to touch a terminal or Git itself. So there is certainly enablement this tool brings to some people.

rikatee wrote at 2020-11-06 17:43:26:

That's good insight, thanks :)

bluntfang wrote at 2020-11-06 14:40:07:

>I've been in a few organization with hundreds of repos. It's can be a pain to upgrade libraries.

Sure, configuration management is hard, it doesn't mean you don't have to do it. If anything, in an organization that big, you should have someone creating a process for managing dependency upgrades. There will always be critical security vulnerabilities and other use cases for package upgrades that need to be patched across all of your repos.

rikatee wrote at 2020-11-06 15:01:54:

In general I agree, but specifically for this: It does mean you don't have to do it _for this particular feature_. If we can "reduce hard" then that's a good thing. It's one less configuration to manage.

bluntfang wrote at 2020-11-06 15:48:16:

But if you have to do it sometimes, and you're proactive about process and automation, not only is this good practice for quickly patching security vulnerabilities, it will help you solidify your configuration management processes and automation, because at the end of the day, you're going to have to do it.

The suggestion that you don't have to do it for this particular feature is kicking the can down the road.

kvalekseev wrote at 2020-11-06 16:42:13:

I wrote

https://github.com/kalekseev/django-extra-checks

that do exactly this using builtin django checks framework, it has a couple of drawbacks though:

1) Django checks doesn't provide a good way to disable checks for some lines.

2) Parsing AST is quite slow and django check runs on every dev server reload.

rikatee wrote at 2020-11-06 12:11:11:

Django Doctor suggests code improvements in your pull requests, so you get better code with no extra effort.

What will Django Doctor say about your code? Check now for free at

https://django.doctor

If you dig this then you may like to install the bot on your GitHub pull requests

https://github.com/marketplace/django-doctor/

scrollaway wrote at 2020-11-06 22:48:40:

Ran it on a repo of mine, looks like it thinks TextField is the same as CharField in some cases:

https://django.doctor/dj-stripe/dj-stripe

rglullis wrote at 2020-11-06 14:24:46:

Is this based on some Django linter tool or some kind of plugin I am not aware of?

rikatee wrote at 2020-11-06 14:46:15:

Nope, all 100% pure custom code.

The closest to existing tools are:

https://github.com/lamby/django-lint/

https://github.com/PyCQA/pylint-django

but- you have to set those up yourself in a CI and update them manually and not have the cool feature like PR integration, "fix it for me" (you would have to fix it yourself like a barbarian), the slick companion website with the beautiful blue progress bar :)

With Django Doctor you just click "install on GitHub" and boom, you get updates at no effort, and if you like the suggestion Django Doctor makes you just click commit

ninjapenguin54 wrote at 2020-11-06 14:05:47:

Empty string and none are not the same or in any way redundant.

Linters belong in ci. Humans belong in code review.

I can actually explain to a human why my text field has a default of none.

VWWHFSfQ wrote at 2020-11-06 14:14:16:

It's a code-smell in Django, but I agree that this kind of thing shouldn't be "auto-fixed" by a bot. There are perfectly valid reasons why you might want to eschew the convention.

> Avoid using null on string-based fields such as CharField and TextField. If a string-based field has null=True, that means it has two possible values for “no data”: NULL, and the empty string. In most cases, it’s redundant to have two possible values for “no data;” the Django convention is to use the empty string, not NULL. One exception is when a CharField has both unique=True and blank=True set. In this situation, null=True is required to avoid unique constraint violations when saving multiple objects with blank values.

[0]

https://docs.djangoproject.com/en/3.1/ref/models/fields/#nul...

theptip wrote at 2020-11-06 20:34:00:

Unfortunately it's tricky to add new fields in a zero-downtime fashion without making them nullable. Django does not store defaults in the DB (this is surprising, and the opposite of the Rails approach), and this means that if you upgrade your DB first, until you've upgraded your application all writes to your model are going to fail because the (down-level) application code doesn't know it has to specify the (up-level) DB field.

From chatting to folks, most applications just YOLO this, and accept downtime on every such DB operation; you might not even notice that this is happening at first because you'd need to have a create _during_ your DB migration. It'll bite you sooner or later though. I'm sure for many apps a client-side retry is totally acceptable, assuming it's safe to do so.

rikatee wrote at 2020-11-06 14:22:00:

Perhaps I need to clarify "auto fix" - the dev still needs to click "commit" on the suggestion. Given a PR with 3 Django Doctor suggestions, 2 could be ignored and only 1 committed by the dev if they choose.

eli wrote at 2020-11-06 15:58:05:

Sure, but if it's outside the usual convention it should be explained with a comment. The comment can also include a command to tell the linter to ignore it.

rikatee wrote at 2020-11-06 14:20:15:

Bear in the comments are "food for thought" and aren't aimed at blocking merge. In the review you can pick which suggestions you want and can ignore the stuff that does not add value to you.

Soon we will be adding a config file so you can mute the things that ar vexatious

tmarice wrote at 2020-11-06 14:18:22:

In most cases they are synonymous. If they're not, you are allowed to disregard the bot's suggestion.

swiley wrote at 2020-11-06 14:09:20:

I hate things that get this wrong so much.

rikatee wrote at 2020-11-06 15:22:55:

I'm sorry swiley :'(

harel wrote at 2020-11-06 21:13:41:

I've been using it for a while and it works really well. It manages to be helpful and not annoying, which is a fine line to tread.

Rotten194 wrote at 2020-11-06 19:54:03:

Is it possible to use on a private repository? It says "Free for public repositories" but I can't find any information about a non-free option, the github page seems to only list the free tier. Is it not available yet?

rikatee wrote at 2020-11-06 23:02:01:

I'm working on private repo support. chat with me at django.doctor if you want to get in on the private beta

ivan_ah wrote at 2020-11-06 14:05:49:

This is pretty good. Found some "Nullable but not blank=True" errors which were hidden because using mostly as an API, but would have had to fix to use the Django admin or forms.

Also suggested reordering of methods within the class, which could be a nice convention to follow for large projects.

rikatee wrote at 2020-11-06 15:11:08:

Glad to be of service :)

mrlinx wrote at 2020-11-06 12:55:43:

I'm not one to install these bots on the company repos. Is there a way to run this offline?

rikatee wrote at 2020-11-06 14:24:27:

Unfortunately not. It's aimed at SaaS so users gain some convenience but lose a bit of other things :)

bredren wrote at 2020-11-06 14:44:24:

Might be nice to have option to use this as a service backing a GitHub Action.

Sort of an extra checking step to my CI.

rikatee wrote at 2020-11-06 15:05:12:

Your wish is granted kind of:

https://github.com/marketplace/django-doctor/

It's a github app, rather than an action. Actions still require you to update a yaml file to use.

Regarding CI: I don't think Django Doctor should block or fail builds. Django Doctor looks suggests improvements, but it's up to the dev to decide to commit them. Plus, no bot should be commiting changes without dev approval. Hello Skynet!

bredren wrote at 2020-11-06 20:12:06:

I saw it is an app, though I don’t see a problem with being able to lint feedback / warnings using an action.

For example, I have a project where I pass back CI info to a slack channel so the dev can look at things easy and go to relevant links based on the notification.

If I was committing a feature branch and DD had a suggestion it would be fine to know at time of commit.

I agree it shouldn’t make it commit changes in this workflow, though. Just point them out.

zachh wrote at 2020-11-06 17:07:54:

This looks really great! Is there a privacy policy?

rikatee wrote at 2020-11-06 17:46:56:

There will be shortly.

In summary:

- we dont' collect ANY personal data because we don't need ay. we don't even have a database, We can't misuse your persona data if we don't have it!

- we don't create any non-essential cookies on browser

- your code is deleted as soon as it's checked

- the servers are hosted in USA/London

michaelmior wrote at 2020-11-06 13:51:11:

I think it would help to somehow edit the title to add that this is exclusive to Django. Looks cool though! Gave some reasonable suggestions for one of my projects :)

rikatee wrote at 2020-11-06 14:23:10:

Thanks for the feedback, and glad you found it useful :)