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

Cabinet memberships documentation #15616

Merged
merged 2 commits into from
Mar 1, 2017
Merged

Conversation

chrismytton
Copy link
Contributor

@chrismytton chrismytton commented Feb 28, 2017

What does this do?

Updates the documentation for the cabinet memberships methods that was added in #15609 and moves it out of the README and into lib/person_cards.rb.

Why was this needed?

Originally we thought this code might be useful to outside people, as it was augmenting the everypolitician-ruby gem. Now though, we've decided that it's best for the functionality to live in some viewer-sinatra specific classes for now, until we have a better idea of how it all fits together at least. So this moves the docs to a more logical place.

Relevant Issue(s)

Extracted from #15615

Follows on from #15609

Replaces #15612

Part of #24

Implementation notes

This uses the YARD @example documentation tag, which render the example code out in the docs, see screenshot below.

Screenshots

yard-person-card

Notes to Reviewer

This is just the documentation for now, the implementation will come as a separate pull request.

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15616 February 28, 2017 07:53 Inactive
@chrismytton chrismytton requested a review from tmtmtmtm February 28, 2017 07:53
@chrismytton chrismytton force-pushed the cabinet-memberships-documentation branch from ddf3c73 to 46db2b9 Compare February 28, 2017 08:28
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15616 February 28, 2017 08:28 Inactive
Copy link
Contributor

@tmtmtmtm tmtmtmtm left a comment

Choose a reason for hiding this comment

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

Probably best to put a PR through to rename memberships to legislative_memberships first before this one.

# @return [Array<EveryPolitician::Popolo::Membership>]
def memberships
def legislative_memberships
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like it belongs here…

@chrismytton
Copy link
Contributor Author

Probably best to put a PR through to rename memberships to legislative_memberships first before this one.

Have pulled that change into #15619

@chrismytton chrismytton force-pushed the cabinet-memberships-documentation branch from 46db2b9 to 043a493 Compare February 28, 2017 13:41
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15616 February 28, 2017 13:41 Inactive
This documents the new (and currently unimplemented)
`PersonCard#cabinet_memberships` method. This method will be used to
return the cabinet memberships for the current person card.
These docs were originally put here when we thought we'd be adding some
bits that would be more useful for external people. As it turns out
we've ended up adding the logic to mostly internal classes for now, so
we're documenting that in the classes themselves.
@chrismytton
Copy link
Contributor Author

Renaming PersonCard#memberships has gone through separately in #15620, so this change is now just moving the cabinet memberships docs out of the README.

Copy link
Contributor

@tmtmtmtm tmtmtmtm left a comment

Choose a reason for hiding this comment

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

👍

@tmtmtmtm tmtmtmtm merged commit 8580485 into master Mar 1, 2017
@tmtmtmtm tmtmtmtm removed the 3 - WIP label Mar 1, 2017
@tmtmtmtm tmtmtmtm deleted the cabinet-memberships-documentation branch December 16, 2017 16:56
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.

2 participants