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

Refine PR Builder Code #5933

Merged

Conversation

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Jul 16, 2019

This PR Will work on improving the code for PR builder by updating the code suggested in code reviews

@saadmk11 saadmk11 added this to In progress in Pull Request Builder via automation Jul 16, 2019
@saadmk11 saadmk11 requested review from humitos and ericholscher and removed request for humitos Jul 16, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Solid changes, just a couple questions.


if external_version:
if not created:
Copy link
Member

@ericholscher ericholscher Jul 16, 2019

I'd switch this logic around, and do if created. It just reads a bit cleaner.


if external_version:
if not created:
# identifier will change if there is a new commit to the Pull/Merge Request
if external_version.identifier != identifier:
external_version.identifier = identifier
external_version.save()
Copy link
Member

@ericholscher ericholscher Jul 16, 2019

We should probably log in this case too (eg. updated external version)

(<a data-bind="attr: {href: commit_url}"><span data-bind="text: commit">{{ build.commit }}</span></a>)
{% else %}
(<span data-bind="text: commit">{{ build.commit }}</span>)
Copy link
Member

@ericholscher ericholscher Jul 16, 2019

Does this break the JS code that tries to update commit_url if it doesn't exist? I don't know how knockout.js handles this, but we might also need to check for it on that side.

Copy link
Member Author

@saadmk11 saadmk11 Jul 16, 2019

Okay I'll check this :)

Copy link
Member Author

@saadmk11 saadmk11 Jul 16, 2019

I have checked. there is no error on the browsers console window. and commit_url does always exist. but set to None when conditions are not met.

@@ -578,18 +578,21 @@ def run_build(self, docker, record):

if self.build_env.failed:
self.send_notifications(self.version.pk, self.build['id'])
# send build failure status to git Status API
# if the build failed, send build failure status to git Status API
# to show status report on the providers pull/merge request UI.
Copy link
Member

@ericholscher ericholscher Jul 16, 2019

This is still just explaining what the code does. We can just remove these comments, we don't need to explain anything here.

if created:
log.info(
'(Create External Version) Added Version: [%s] ', ' '.join(
external_version.slug
Copy link
Member

@ericholscher ericholscher Jul 16, 2019

Why are we calling join on a slug?

Copy link
Member Author

@saadmk11 saadmk11 Jul 16, 2019

Opps! I was doing something else forgot to remove it sorry.

@ericholscher ericholscher merged commit ea09b41 into readthedocs:gsoc-19-pr-builder Jul 16, 2019
Pull Request Builder automation moved this from In progress to Done Jul 16, 2019
@saadmk11 saadmk11 deleted the refine-pr-builder branch Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants