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

subclassing API #29

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

subclassing API #29

wants to merge 6 commits into from

Conversation

xi
Copy link
Contributor

@xi xi commented May 12, 2014

While working on a project using Flask-FlatPages I found myself wanting to exclude some pages (e.g. because they had metadata like published: false or date: 2040-03-12). I also wanted to normalize the metadata (e.g. allowing both category and categories).

My approach for implementing this was to subclass FlatPages. I guess this might be a common usecase so providing default hooks for this kind of functionality seemd like a good idea. So now I added to methods _is_excluded and _process that do nothing by default but can be overwritten by subclasses.

However, there is an issues with this: Because both methods are private they are not included in the autogenerated documentation.

@playpauseandstop
Copy link
Collaborator

Good intention, but I think these methods should be public, why protect them to reuse on inherited classes?

I'd like to rename them to is_excluded_page and process_page methods, what do you think?

@xi
Copy link
Contributor Author

xi commented May 17, 2014

I liked the idea of appending '_page' to the function names and did so in 7e5bf8b. I also managed to include them in the sphinx documentation ( ccdbaf4).

I thought about it and I still think that these functions should be protected. When using an instance of FlatPages (or a subclass) both functions are essentially useless: is_excluded_page would always return False because I only get to see pages that are not excluded. Likewise, process_page has already been applied to each page I get to see.

I think both funtions are internal implementation details. They should not clutter the public API of FlatPages.

@playpauseandstop
Copy link
Collaborator

Also didn't note this pull request while making 0.6, moved to 1.0

@playpauseandstop playpauseandstop added this to the 1.0 milestone Feb 12, 2015
Conflicts:
	flask_flatpages/flatpages.py
@xi
Copy link
Contributor Author

xi commented Feb 21, 2015

I merged master and resolved one merge conflict.

@playpauseandstop playpauseandstop modified the milestones: 0.7, 1.0 Jun 5, 2015
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.

None yet

2 participants