Refactor by Jarrod-Fivium · Pull Request #1 · ToukanLabs/TAIPAN

Jarrod-Fivium · 2017-08-10T07:28:04Z

No description provided.

tombasche · 2017-08-10T07:31:51Z

    pass


+@admin.register(SuiteDetail)


huh I didn't know you could add a decorator to do an admin.register...

neat!

tombasche · 2017-08-10T07:35:11Z

-  height: 100%;
-  width: 100%;
-  position: absolute; }
+  background-color: #FEFEFE !important; }


#COVFEFE is a different color

tombasche · 2017-08-10T07:35:37Z

@@ -46,24 +46,16 @@ html {
  font-family: "Trebuchet MS", sans-serif; }


The closing curly brace should be on the next line

this is autogenerated via sass

tombasche · 2017-08-10T07:37:33Z

    {% load static %}
    <link rel="stylesheet" type="text/css" href="{% static 'projects/css/style.css' %}">
+    <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css"
+          integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u" crossorigin="anonymous">


Huh ... good use of html5 integrity / crossorigin attributes!

sarcasm? I copypasta'd

Come on at least take credit for it! Do you understand what they do? Here's a good explanation. Essentially it's like a simple way of defining CORS (cross-origin resource sharing) as well as verifying that nothing is tampering with your CDN code.

tombasche · 2017-08-10T07:39:56Z

+                <th class="left-align">Result</th>
+                <th class="right-align">Actions</th>
+            </tr>
+            {% for btd in tests %}


try and be more verbose in variable names. I have no idea what btd is!

all of these have been renamed

tombasche · 2017-08-10T07:42:05Z

+                <th class="left-align">Description</th>
+                <th class="right-align">Actions</th>
+            </tr>
+            {% for bid in builds %}


again, bid isn't overly helpful. try to use the singular. something like the following is the pythonic way:

for instance in instances: instance.do_your_thing()

tombasche · 2017-08-10T07:42:51Z

+                <th class="left-align">Required?</th>
+                <th class="right-align">Actions</th>
+            </tr>
+            {% for td in tests %}


I feel like td could be confused for table data. again, probably best with test for this one.

tombasche · 2017-08-10T07:49:28Z

@@ -0,0 +1,306 @@
+from django.views.generic import ListView, DetailView, CreateView, UpdateView, DeleteView
+from .models import *


again, avoid import *

tombasche · 2017-08-10T07:51:12Z

+from .models import *
+from django.http import HttpResponseRedirect
+from datetime import datetime
+import reversion


The order of imports should be (according to PEP8):

standard library imports
related third party imports
local application/library specific imports

You should put a blank line between each group of imports.

tombasche · 2017-08-10T07:51:54Z

+# Create your views here.
+class IndexView(ListView):
+    template_name = 'projects/index.html'
+    context_object_name = 'projects'


again, this should reference a global specified in settings.

tombasche · 2017-08-10T07:52:16Z

+# DETAILS
+class ProjectDetailView(DetailView):
+    model = ProjectDetail
+    context_object_name = 'pd'


What's wrong with just project rather than pd? Try and be descriptive in variable names.

Actually come to think of it - why have you got Detail after every model?

tombasche · 2017-08-10T07:54:17Z

+            detail.save()
+            # Create a test suite for the bundle
+        with reversion.create_revision():
+            suite = SuiteDetail(bundle=detail, name=detail.name + ' Testing Suite', description='Test suite for ' + detail.name)


rather than adding together strings use the format syntax:

tom = 'tom' new_string = '{} is a cool dude'.format(tom)

yeah I wasn't thinking in python

If you're in Python 3.6 you can do f'{tom} is a cool dude'

that's kinda like the es6 backtick stuff in javascript then. neat

Sorry for the big commit

Added the reporting page with chart.js. Some css changes

Refactor

f613665

tombasche reviewed Aug 10, 2017

View reviewed changes

tombasche suggested changes Aug 10, 2017

View reviewed changes

Jarrod-Fivium added 2 commits September 7, 2017 11:07

Refactor and adding of landing page

03de51c

Sorry for the big commit

Style updates, more pages

3a4bdc3

Added the reporting page with chart.js. Some css changes

		@@ -46,24 +46,16 @@ html {
		font-family: "Trebuchet MS", sans-serif; }

		@@ -0,0 +1,306 @@
		from django.views.generic import ListView, DetailView, CreateView, UpdateView, DeleteView
		from .models import *

		pass


		@admin.register(SuiteDetail)

Conversation

Jarrod-Fivium commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants