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

Split ComponentDoc into ComponentDoc and ComponentDocResolver #34

Merged
merged 2 commits into from Aug 18, 2017
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Aug 17, 2017

A small refactor to switch from structs to classes.

Specifically:

Addresses some of @alecgibson’s original review comments in #1 (comment)

fofr added 2 commits Aug 16, 2017
Split logic for retrieving component documentation from the component
documentation itself
* Allows path to be easily changed
* Allows this path to be monkey patched by govuk_component_guide
* DRYs up templates
@fofr fofr requested a review from alecgibson Aug 17, 2017
Copy link

@alecgibson alecgibson left a comment

I'm not so sure about the monkey-patching pattern here. It's fragile and requires intimate knowledge of the internals of the class. If we think this is going to happen quite a lot, wouldn't it be nicer to configure the engine with a custom subclass of an abstract ComponentDocResolver? (Obviously with a sensible default concrete class)

Dir[doc_files].sort.map { |file| parse_documentation(file) }
end

def fetch_component_doc(id)

This comment has been minimized.

@alecgibson

alecgibson Aug 18, 2017

So every time we try to access documentation we have to perform multiple file operations? Feels a little bit icky; could potentially memoise this stuff at a static level? Although given that this is all just internal code used by developers, maybe we don't have to worry about it too much.

This comment has been minimized.

@fofr

fofr Aug 18, 2017
Author Contributor

I want to make it faster, and memoising makes sense. As this is predominantly a tool for developing components we need to balance that with the problem of too much caching hiding local changes.

This comment has been minimized.

@alecgibson

alecgibson Aug 18, 2017

Makes sense to me. Not sure we'd notice much of a performance change, anyway.

component[:body],
component[:accessibility_criteria],
fixtures)
class ComponentDoc

This comment has been minimized.

@alecgibson

alecgibson Aug 18, 2017

Why have we moved away from a Struct? It looks like we've just wound up with more boilerplate code. To make it immutable? We could use something like immutable-struct?

@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 18, 2017

If we think this is going to happen quite a lot

I see it happening once, for static components that are different. But I like your idea of a subclass component resolver within the gem. I've been monkey patching as a proof of concept. We're also considering retiring govuk-component-guide and putting the gem in static. That would require something custom because static can't request from slimmer.

@alecgibson
Copy link

@alecgibson alecgibson commented Aug 18, 2017

I see it happening once, for static components

If that's the case, then do it however you think is best - if it's just for one special case, a whole inheritance system may be overkill.

Copy link

@alecgibson alecgibson left a comment

Sounds fine to me unless you want to do the immutable-struct thing (although I've not really seen it used around GOV.UK, so may be a little unusual anyway).

@fofr fofr merged commit 69535e4 into master Aug 18, 2017
1 check passed
1 check passed
continuous-integration/jenkins/branch This commit looks good
Details
@fofr fofr deleted the structs-out branch Aug 18, 2017
@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 18, 2017

Thank you for the review @alecgibson ⭐️

fofr added a commit that referenced this pull request Aug 25, 2017
* Add aXe accessibility testing javascript (PR #33)
* Mark strings in YAML fixtures as HTML safe (PR #36)
* Refactor internal structs to use classes (PR #34)
@fofr fofr mentioned this pull request Aug 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.