Skip to content

Conversation

antgonza
Copy link
Member

No description provided.

@antgonza antgonza changed the title 2022.07 [WIP] 2022.07 Jun 28, 2022
@coveralls
Copy link

coveralls commented Jun 28, 2022

Coverage Status

coverage: 92.837% (-0.01%) from 92.848%
when pulling 51211dd on antgonza:2022.07
into 9bba2bd on qiita-spots:dev.

@antgonza antgonza changed the title [WIP] 2022.07 2022.07 Jul 1, 2022
@antgonza antgonza requested a review from charles-cowart July 1, 2022 13:11
Copy link
Contributor

@charles-cowart charles-cowart left a comment

Choose a reason for hiding this comment

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

Everything else looks good. One possible change and one request for clarification.

self.command.name, self.id))
message = ('New status: %s' % (new_status))
qdb.util.send_email(self.user.email, subject, message)
if value not in ('waiting'):
Copy link
Contributor

Choose a reason for hiding this comment

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

since there's only one value, shouldn't it be if value != 'waiting'? Also, without a ',' this will come back as type str, according to the interpreter.

ignore_commands = ('Validate', 'complete_job',
'release_validators')
if self.command.name not in ignore_commands:
subject = 'Job status change: %s (%s)' % (
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to use %s myself when I have the choice. Just want to confirm that it's okay to use %s and not f'{}' in new code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed this is a matter of preference and what works well for placating flake8.

Copy link
Contributor

@charles-cowart charles-cowart left a comment

Choose a reason for hiding this comment

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

Approved!

@charles-cowart charles-cowart merged commit e6aa09d into qiita-spots:dev Jul 2, 2022
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.

3 participants