Skip to content

Conversation

jaap3
Copy link
Contributor

@jaap3 jaap3 commented Aug 27, 2018

This makes it possible to create a release without installing the Box fixtures.

This should be perfectly safe as the box content is set immediately after getting/creating the box.

@jaap3
Copy link
Contributor Author

jaap3 commented Aug 27, 2018

Looking at this again I think this could even be changed to update_or_create. Let me know if that's preferred.

Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

You're right, changing the code to use update_or_create() would be better.

@jaap3 jaap3 force-pushed the box-get-or-create branch from 99caa5d to 3f9593a Compare August 27, 2018 11:19
@jaap3
Copy link
Contributor Author

jaap3 commented Aug 27, 2018

I've updated the PR to use update_or_create, I've also changed the use of Box.objects.get / Box.objects.get_or_create in other places

Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

This looks like a good cleanup, thanks! I only have a comment about using trailing commas after the last item of the dict.

blogs/parser.py Outdated
label='supernav-python-blog',
defaults={
'content': rendered_box,
'content_markup_type': 'html'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please add a trailing comma.

label='supernav-python-downloads',
defaults={
'content': content,
'content_markup_type': 'html'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please add a trailing comma.

label='download-sources',
defaults={
'content': source_content,
'content_markup_type': 'html'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please add a trailing comma.

label='homepage-downloads',
defaults={
'content': content,
'content_markup_type': 'html'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please add a trailing comma.

label='supernav-python-success-stories',
defaults={
'content': content,
'content_markup_type': 'html'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please add a trailing comma.

Jaap Roes added 4 commits August 28, 2018 09:38
This makes it possible to create a release without installing the Box fixtures
Remove unnecessary box creation from test cases
@jaap3 jaap3 force-pushed the box-get-or-create branch from 3f9593a to fbd89f2 Compare August 28, 2018 07:39
@jaap3
Copy link
Contributor Author

jaap3 commented Aug 28, 2018

I've added the trailing commas.

On a related note: It might be interesting to run black on this codebase at some point. That would fix a lot of existing code style inconsistencies.

@berkerpeksag berkerpeksag merged commit 97de091 into python:master Aug 28, 2018
@berkerpeksag
Copy link
Member

Thanks!

On a related note: It might be interesting to run black on this codebase at some point. That would fix a lot of existing code style inconsistencies.

Sorry, I dislike some of the style choices of black :) Also, we don't accept cosmetic-only patches in CPython, so I'd like to follow that rule in python.org as well.

@jaap3 jaap3 deleted the box-get-or-create branch August 28, 2018 10:50
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.

2 participants