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

blacken-docs #8906

Merged
merged 6 commits into from Oct 13, 2023
Merged

blacken-docs #8906

merged 6 commits into from Oct 13, 2023

Conversation

jvzammit
Copy link
Contributor

@jvzammit jvzammit commented Mar 15, 2023

blacken-docs

Description

This PR shows the changes made by applying blacken-docs to the project's docs.

It's objective is to show:

  1. The changes required for the command to run without errors -- first commit
  2. The changes that would be made by the command -- third commit

The second commit adds blacken-docs step to the precommit hook config file. This breaks on CI right now. And I couldn't replicate the issue locally. Help very much appreciated.

Update: Fixed in fourth and fifth commit.

@@ -213,14 +214,14 @@ Note that the `basename` is provided by the router during `ViewSet` registration

Using the example from the previous section:

```python
>>> view.reverse_action('set-password', args=['1'])
```pycon
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is pycon a valid syntax highlighter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of this. Thanks :D

Comment on lines 34 to 36
content = {
'user_feed': request.user.get_user_feed()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can do

 content = {
            'user_feed': request.user.get_user_feed(),
     }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something changed by the command, not a manual change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comma at the end to make it look identical to old version

Comment on lines 45 to 47
content = {
'user_feed': request.user.get_user_feed()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

content = {
            'user_feed': request.user.get_user_feed(),
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something changed by the command, not a manual change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comma at the end to make it look identical to old version

Comment on lines 55 to 58
content = {
'title': 'Post title',
'body': 'Post content'
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

content = {
            'title': 'Post title',
            'body': 'Post content',
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something changed by the command, not a manual change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add commas at the end to make it look identical to old version

@jvzammit
Copy link
Contributor Author

@baseplate-admin Thank you for the initial review.

Please notice the structure of the commits -- third commit changes are by the command itself -- i.e. I didn't make those changes.

So what's for review is really the remaining changes (please note the commit message).

@baseplate-admin
Copy link
Contributor

Please notice the structure of the commits -- third commit changes are by the command itself -- i.e. I didn't make those changes.

Yep i am aware that it was automatically changed by black. But i am asking to add a comma at the end to make the block look how it used to look.

@jvzammit
Copy link
Contributor Author

@baseplate-admin OK I understand, thank you for the feedback, will change and re-run locally before pushing anything -- will try to get to it asap. Thanks!

@jvzammit
Copy link
Contributor Author

@baseplate-admin Changes you suggested work, thanks 👍

Copy link
Contributor

@baseplate-admin baseplate-admin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small edit and everything else looks good :D

Comment on lines 43 to 47
@method_decorator(
vary_on_headers(
"Authorization",
)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks better, dont you think so?

@method_decorator(vary_on_headers("Authorization"))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baseplate-admin Absolutely, I didn't want this PR to touch any of this though, that's why I only applied changes that I had to. But will push this, it's a very minor change. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baseplate-admin Pushed (squashed change into previous commit).

Copy link
Contributor

@baseplate-admin baseplate-admin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM....

Thanks a bunch @jvzammit

@stale
Copy link

stale bot commented Jun 11, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 11, 2023
@jvzammit
Copy link
Contributor Author

@baseplate-admin Thank you for approving. Can this be merged?

@stale stale bot removed the stale label Oct 13, 2023
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid. 🖤

@tomchristie tomchristie merged commit e794e5e into encode:master Oct 13, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants