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

Alter resources to use class based generators #636

Merged
merged 29 commits into from Oct 24, 2017

Conversation

3 participants
@JackMorganNZ
Member

JackMorganNZ commented Oct 23, 2017

This pull request changes how resources are created, changing from basic Python modules to using a generator class inheriting from a base resource generator class.

Changes to do once approved:

  • Update documentation

JackMorganNZ added some commits Oct 19, 2017

Add method to process requested options for resources
This method converts any boolean values "True" in the
query string to the boolean type equivalent.
@codecov

This comment has been minimized.

codecov bot commented Oct 23, 2017

Codecov Report

Merging #636 into develop will increase coverage by 0.29%.
The diff coverage is 91.78%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #636      +/-   ##
===========================================
+ Coverage    84.93%   85.23%   +0.29%     
===========================================
  Files           81       83       +2     
  Lines         2224     2215       -9     
  Branches       267      270       +3     
===========================================
- Hits          1889     1888       -1     
+ Misses         269      262       -7     
+ Partials        66       65       -1
Impacted Files Coverage Δ
...ed/resources/views/ParityCardsResourceGenerator.py 100% <100%> (ø)
...plugged/resources/views/ArrowsResourceGenerator.py 100% <100%> (ø)
...resources/views/LeftRightCardsResourceGenerator.py 100% <100%> (ø)
csunplugged/utils/get_resource_generator.py 100% <100%> (ø)
...gged/resources/views/JobBadgesResourceGenerator.py 100% <100%> (ø)
csunplugged/resources/models.py 100% <100%> (ø) ⬆️
...resources/views/SearchingCardsResourceGenerator.py 100% <100%> (ø)
...gged/resources/views/PianoKeysResourceGenerator.py 100% <100%> (ø)
...plugged/utils/errors/QueryParameterMissingError.py 100% <100%> (ø)
...sources/views/BinaryCardsSmallResourceGenerator.py 100% <100%> (ø)
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22e52e2...0fb2837. Read the comment docs.

@jordangriffiths01

This is super cool!

Just a few minor things to change or discuss :)

import importlib
def import_resource_generator(generator_module, requested_options=None):

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 23, 2017

Contributor

This function returns an instantiated ResourceGenerator object, so "import_" is somewhat misleading in the name. Perhaps "get_"?

def process_requested_options(self, requested_options):
"""Convert requested options to usable types.

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 23, 2017

Contributor

Missing args: section in docstring

This comment has been minimized.

@JackMorganNZ

JackMorganNZ Oct 23, 2017

Member

There are no arguments? Unless you want me to document self? Was looking at wrong section, will fix.

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

what about requested_options ?

This comment has been minimized.

@JackMorganNZ
@@ -4,7 +4,7 @@
from utils.bool_to_yes_no import bool_to_yes_no
def resource_valid_test_configurations(valid_options):
def resource_valid_test_configurations(valid_options, header_text=True):
"""Return list of all possible valid resource combinations.
Args:

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 23, 2017

Contributor

Add header_text to args section

This comment has been minimized.

@JackMorganNZ
for option in self.valid_options.keys():
values = requested_options.getlist(option)
if not values:
raise Http404("{} parameter not specified.".format(option))

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 23, 2017

Contributor

This feels like we're throwing Http404 at a very low level. Maybe we should consider defining some specific Exception types for these errors, and then catch these and raise 404 in views/generate_resource_pdf.py?

This comment has been minimized.

@JackMorganNZ

JackMorganNZ Oct 23, 2017

Member

I like it! Will add this next.

- Raises 404 is option given with invalid value.
Returns:
QueryDict of converted requested options (QueryDict).

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 23, 2017

Contributor

What's the reason for using a QueryDict rather than a normal python dictionary? It feels like the generator class shouldn't need to have any coupling to the request/response framework.

thumbnail-static-path: img/resources/binary-cards/thumbnail.png
copies: false
binary-cards-small:
name: Binary Cards (Small)
webpage-template: resources/binary-cards-small.html
generation-view: binary_cards_small.py
webpage-template: resources/binary-cResourceGeneratorards-small.html

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

stray string in the middle of this template name

class BarcodeChecksumPosterResourceGenerator(BaseResourceGenerator):
"""Class for Grid resource generator."""
additional_valid_options = {

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

why does this live here and not outside of the class/what is this magic?

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 23, 2017

Contributor

It's a class variable, available at BarcodeChecksumPosterResourceGenerator.additional_valid_options. It's the same for all instances of the class, hence why it is set here rather than init.

image_file = "col_binary_lightbulb_off.png"
lightbulb = Image.open(os.path.join("static/img/topics/", image_file))
(width, height) = lightbulb.size
SCALE_FACTOR = 0.6

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

should this be listed outside the class with the other constants?

draw.line([(IMAGE_SIZE_X - 1, 0), (IMAGE_SIZE_X - 1, IMAGE_SIZE_Y)], fill=LINE_COLOUR, width=LINE_WIDTH)
for y_coord in range(0, IMAGE_SIZE_Y, BOX_SIZE):
draw.line([(0, y_coord), (IMAGE_SIZE_X, y_coord)], fill=LINE_COLOUR, width=LINE_WIDTH)
draw.line([(0, IMAGE_SIZE_Y - 1), (IMAGE_SIZE_X, IMAGE_SIZE_Y - 1)], fill=LINE_COLOUR, width=LINE_WIDTH)

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

the resources get away with putting a lot on one line when they should perhaps be spread across multiple, but lines 27, 28, 30 and 31 here really feel like a bit much....maybe I'm just being fussy, idk.

Args:
request: HTTP request object (HttpRequest).
resource: Object of resource data (Resource).

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

stray args

if prefilled_values == "blank":
range_text = "blank"
else:
SUBTITLE_TEMPLATE = "{} - {} to {}"

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

should this be put with the other constant before the class?

"""Return number range tuple for resource.
Args:
request: HTTP request object (HttpRequest).

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

stray args

- Raises 404 error is requested option cannot be found.
- Raises 404 is option given with invalid value.
Returns:

This comment has been minimized.

@hayleyavw

This comment has been minimized.

@JackMorganNZ
@@ -4,7 +4,7 @@
from utils.bool_to_yes_no import bool_to_yes_no
def resource_valid_test_configurations(valid_options):
def resource_valid_test_configurations(valid_options, header_text=True):

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

new parameter needs to be added to docstring args

This comment has been minimized.

@JackMorganNZ
@@ -38,7 +38,7 @@ def test_resources_index_with_multiple_resources(self):
"binary-cards",
"Binary Cards",
"resources/binary-cards.html",
"binary_cards.py",
"BinaryCardsResourceGenerator",
)
self.test_data.create_resource(

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

sorting_network.py doesn't exist anymore

This comment has been minimized.

@hayleyavw

hayleyavw Oct 23, 2017

Member

this test passes despite sorting_network.py not existing anymore, does that seem weird do you? I understand why it passes, but it seems wrong that it should.

This comment has been minimized.

@JackMorganNZ

JackMorganNZ Oct 24, 2017

Member

This is because the resource is added with the test loader instead of the resource loader, I've created issue #639 to investigate this.

@hayleyavw

This comment has been minimized.

Member

hayleyavw commented Oct 23, 2017

this is looking so much better now, great work 👍

@JackMorganNZ

This comment has been minimized.

Member

JackMorganNZ commented Oct 23, 2017

Thanks for the reviews team! Great to know it's going in the right direction. I'll keep working on this and ping you for final review.

JackMorganNZ added some commits Oct 24, 2017

If ``additional_valid_options`` are given, the ``subtitle`` property method should be overridden.
The method should display the additional options and also call the parent's subtitle result:
.. function:: subtitle(self)

This comment has been minimized.

@jordangriffiths01

jordangriffiths01 Oct 24, 2017

Contributor

Probably worth including that if subtitle is overloaded by the subclass, it must be a property (i.e. have the property decorator @property and only take one parameter, self).

JackMorganNZ added some commits Oct 24, 2017

@JackMorganNZ JackMorganNZ merged commit 0fb2837 into develop Oct 24, 2017

4 checks passed

codecov/patch 91.78% of diff hit (target 84.93%)
Details
codecov/project 85.23% (+0.29%) compared to 22e52e2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@JackMorganNZ JackMorganNZ deleted the class-based-resources branch Oct 24, 2017

@JackMorganNZ JackMorganNZ referenced this pull request Oct 30, 2017

Merged

Release 2.0.0 alpha.5 #657

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment