pre-commit command ran for humitos/log/sentry branch #3898

humitos · 2018-04-04T03:19:25Z

Logic changes are under #3893

Log only what we need as ERROR when it's something that we need/want to take a look that something could be a bug/issue. There are some ERROR that were replaced by WARNING since they are some failures but that we don't need to take a look and the message shown to the user should be enough for them.

agjohnson

This is bigger than I thought, I have to come back to this.

I left off at environment.py, here are a few changes so far.

agjohnson · 2018-04-05T22:59:37Z

readthedocs/builds/syncers.py

-                        slash=slash,
-                        user=sync_user,
-                        server=server,
+                        path=path, slash=slash, user=sync_user, server=server,


This squashed the continuation

I had sent a comment here, but my internet connection didn't help me:

By adding a , after the target here, we get a one attribute per line formatting automatically by yapf.

There are many changes on these files were we can apply this fix to get the more readable way, but I don't want to do it manually for now.

I think we can easily add the trailing comman once we start using pre-commit in a daily basis.

Anyway, I think we can use this PR to discuss/tweak yapf a little more.

There are things that need manual work. Although, for this particular issue that the trailling comma is missing we could add automatically before calling yapf with this hook: https://github.com/asottile/add-trailing-comma

agjohnson · 2018-04-05T23:00:30Z

readthedocs/builds/syncers.py

-                    path=path,
-                    user=sync_user,
-                    server=server,
+                    host=host, path=path, user=sync_user, server=server,


Squashed continuation here

agjohnson · 2018-04-05T23:01:19Z

readthedocs/core/management/commands/clean_builds.py

+        queryset = (
+            Build.objects.values('project',
+                                 'version').annotate(max_date=Max('date'))
+            .filter(max_date__lt=max_date).order_by('-max_date'))


Previous continuation style was better

agjohnson · 2018-04-05T23:02:27Z

readthedocs/core/views/hooks.py

                data = json.loads(request.POST.get('payload'))
            else:
                data = json.loads(request.body)
            http_url = data['repository']['url']
-            http_search_url = http_url.replace('http://', '').replace('https://', '')
+            http_search_url = http_url.replace('http://',
+                                               '').replace('https://', '')


I'd prefer a different method of breaking this up to multiple lines

humitos · 2018-04-06T01:02:35Z

I re-configure pre-commit (depends on this branch readthedocs/common#2) and re-run it to show what are the news here.

I found one problem though, the definition of the functions is also affected by add-trailing-comma plugin, so I will see if we can disable that.

Anyway, it seems to be too much closer to what you want.

humitos

I'd say that this is very close to what we want.

I noted some points that we should care

humitos · 2018-04-06T01:03:27Z

readthedocs/builds/models.py

+            docroot,
+            filename,
+            source_suffix='.rst',
+            action='view',


Here, the function is affected by add-trailing-comma plugin.

I want to check how to disable this case.

This is not a problem of this plugin, but a combination of add-trailing-comma plus yapf.

When this is detected

def myfunc( a, b, c, d, e, f, argument, larger, here, to, reach, eighty, lines, NOW ):

trailing comma adds a comma in the end, and yapf format it on an argument per line in the next pass.

humitos · 2018-04-06T01:04:45Z

readthedocs/core/views/hooks.py

-            sync_repository.apply_async(
-                args=(version.pk,),
-            )
+            sync_repository.apply_async(args=(version.pk,),)


It seems it gets confused here. Not a big issue, though.

humitos · 2018-04-06T01:05:16Z

readthedocs/core/views/hooks.py

@@ -173,12 +182,16 @@ def github_build(request):  # noqa: D205
    """
    if request.method == 'POST':
        try:
-            if request.META['CONTENT_TYPE'] == 'application/x-www-form-urlencoded':
+            if request.META['CONTENT_TYPE'
+                            ] == 'application/x-www-form-urlencoded':


This is one is terrible.

We could add # yapf: disable here, since it's a special case.

or use a variable for application/x-www... so the line is shorter

humitos · 2018-04-06T01:06:39Z

readthedocs/core/views/hooks.py

-            version = 2 if request.META.get('HTTP_USER_AGENT') == 'Bitbucket-Webhooks/2.0' else 1
+            version = 2 if request.META.get(
+                'HTTP_USER_AGENT',
+            ) == 'Bitbucket-Webhooks/2.0' else 1


This one is also terrible.

There are some _PENALTY settings that we should take a look for this case.

humitos · 2018-04-06T01:09:02Z

readthedocs/payments/forms.py

-            'is_cc_group': True,
-            'fields': []
-        }
+        group = {'is_cc_group': True, 'fields': []}


Here it should have added the , and formatted it as it was.

humitos · 2018-04-06T01:11:26Z

readthedocs/search/parse_json.py

+            if filename in [
+                    'search.fjson',
+                    'genindex.fjson',
+                    'py-modindex.fjson', ]:


Also weird here

humitos

I'd say that this is very close to what we want.

I noted some points that we should care

humitos · 2018-04-07T18:44:45Z

I found one problem though, the definition of the functions is also affected by add-trailing-comma plugin, so I will see if we can disable that.

This is not possible to disable in this plugin. I opened an issue in yapf to see why it's not respecting a setting: google/yapf#551

ericholscher · 2018-05-30T21:11:34Z

Pretty sure we can close this, as it has a million conflicts.

ericholscher · 2018-05-30T21:12:08Z

Actually not sure -- what should we do here?

humitos · 2018-05-30T21:20:54Z

Let's close it.

We still don't have a strict policy regarding how to ues pre-commit. We are not forcing nobody to run this command and because nobody is using it in a daily basis it may need some more work now.

We can come back to this later with more time.

humitos added 3 commits April 2, 2018 22:24

Fix lint issues

40a4ff3

Update rtd-common submodule with yapf config

2f2bd2f

humitos mentioned this pull request Apr 4, 2018

Organize logging levels #3893

Merged

pre-commit run for modified files

b90552b

humitos force-pushed the humitos/auto-lint/sentry branch from 2e89010 to b90552b Compare April 4, 2018 03:26

humitos requested a review from agjohnson April 5, 2018 22:04

agjohnson changed the base branch from humitos/log/sentry to master April 5, 2018 22:58

agjohnson requested changes Apr 5, 2018

View reviewed changes

pre-commit after new configs

9958feb

humitos commented Apr 6, 2018

View reviewed changes

agjohnson added this to the Cleanup milestone Apr 17, 2018

ericholscher closed this May 30, 2018

ericholscher reopened this May 30, 2018

humitos closed this May 30, 2018

humitos deleted the humitos/auto-lint/sentry branch May 30, 2018 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pre-commit command ran for humitos/log/sentry branch #3898

pre-commit command ran for humitos/log/sentry branch #3898

humitos commented Apr 4, 2018

agjohnson left a comment

agjohnson Apr 5, 2018

humitos Apr 6, 2018

agjohnson Apr 5, 2018

agjohnson Apr 5, 2018

agjohnson Apr 5, 2018

humitos commented Apr 6, 2018

humitos left a comment

humitos Apr 6, 2018

humitos Apr 7, 2018

humitos Apr 6, 2018

humitos Apr 6, 2018

humitos Apr 6, 2018

stsewd Apr 6, 2018

humitos Apr 6, 2018

humitos Apr 6, 2018

humitos Apr 6, 2018

humitos Apr 6, 2018

humitos left a comment

humitos commented Apr 7, 2018

ericholscher commented May 30, 2018

ericholscher commented May 30, 2018

humitos commented May 30, 2018

pre-commit command ran for humitos/log/sentry branch #3898

pre-commit command ran for humitos/log/sentry branch #3898

Conversation

humitos commented Apr 4, 2018

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Apr 6, 2018

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

humitos commented Apr 7, 2018

ericholscher commented May 30, 2018

ericholscher commented May 30, 2018

humitos commented May 30, 2018