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

Automatically Get Arguments for Endpoints & Compatibility with Flask-Classy #33

Merged
merged 1 commit into from Jul 23, 2015

Conversation

Projects
None yet
4 participants
@nickw444
Contributor

nickw444 commented Apr 30, 2015

These commits bring the following functionality:

Automatically get arguments for endpoints which take arguments by default

Allows users to use @menu.register_menu on endpoints with routes such as /int:id/endpoint1, /int:id/endpoint2, /int:id/endpoint3, which need to link between each other (say to provide a user profile, with links between sections on a specific user profile.).

To achieve this, see 4112383 @ line 56-63. A URL value preprocessor is used to extract the current endpoint's arguments, and we store these on the variable. We later read these when calling MenuEntryMixin.url(). If _endpoint_arguments_constructor is given, these arguments are ignored entirely. A later commit brings argument filtering, meaning generated urls which do not take the arguments won't have a query string appended. This is done by querying the argspec of a method at runtime. d102c05

Compatibility for using Flask-Classy with this library.

Allows registration via menu.register_flaskview(app, classy_view). Within the Flask View, declare endpoints as menu items using menu.classy_menu_item(path, text, **kwargs), where kwargs is any argument that menu.register would take. It may be a good design choice to rename these two functions to make them less ambiguous to something such as @menu.register_classy_menu(), since @menu.register_menu is the current non-classy alternative.

Adds has_visible_child to check if a child is currently visible for a menu item.

Useful for expanding/contracting menus/sub menus. Simply a recursive call, so use sparingly. (if called with recursive=True)

This pull request includes documentation in the docs folder and has been tested + has additional test cases. Brings coverage up to 99%

@@ -207,6 +207,45 @@ with 3 items while processing a request for `/social/list`.
... assert current_menu.submenu('account.list').active
... current_menu.children
Flask-Classy
===

This comment has been minimized.

@jirikuncar

jirikuncar Apr 30, 2015

Member

please make it the same length like the line above

.. code-block:: python
from flask.ext.classy import FlaskView

This comment has been minimized.

@jirikuncar

jirikuncar Apr 30, 2015

Member

for Flask 1.0 we should rather use flask_<extension> hence from flask_classy import ... and import flask_menu as menu

pass
As you can see, instead of using the `@menu.register_menu` decorator, we use classy_menu_item. All usage is otherwise the same to `register_menu`, however you do not need to provide reference to the blueprint/app.

This comment has been minimized.

@jirikuncar

jirikuncar Apr 30, 2015

Member

please try to keep the line length reasonable

pass
def test_classy_endpoint_on_blueprint(self):
from flask.ext.classy import FlaskView

This comment has been minimized.

@jirikuncar

jirikuncar Apr 30, 2015

Member

s/flask\.ext\.classy/flask_classy/

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Apr 30, 2015

@jirikuncar Just pushed the changes you requested. Let me know if there's anything else that may be required. Cheers.

@coveralls

This comment has been minimized.

coveralls commented Apr 30, 2015

Coverage Status

Coverage increased (+4.05%) to 99.44% when pulling a007404 on nickw444:master into 0698254 on inveniosoftware:master.

@jirikuncar

This comment has been minimized.

Member

jirikuncar commented Apr 30, 2015

@nickw444 we are also trying to keep our git history well organized. Can you look to our commit message style and squash some of your commits? Thanks

PS: I will have closer look to this PR next week. Are you in hurry for integration?

.. code-block:: python
from flask.ext import menu

This comment has been minimized.

@jirikuncar

jirikuncar Apr 30, 2015

Member

please make the replacement s/flask.ext./flask_/ globally ...

This comment has been minimized.

@nickw444

nickw444 Apr 30, 2015

Contributor

@jirikuncar Whilst i'm at it, would you like this done for the entire document, rather than just my additions? (Not sure if you mean globally by my changes, or the entire document)

)
def classy_menu_item(path, text, **kwargs):

This comment has been minimized.

@jirikuncar

jirikuncar Apr 30, 2015

Member

does it really depends on classy? can we find some more generic name?

This comment has been minimized.

@nickw444

nickw444 Apr 30, 2015

Contributor

It would be nice to find a more generic name, however the URL endpoints that classy generates (and this implementation relies on) probably aren't too portable. Classy generates it's endpoints as 'Classname:methodname', see https://github.com/inveniosoftware/flask-menu/pull/33/files#diff-3839e0a4ca06ff71b86a56b51eafdf11R373.

@jirikuncar

This comment has been minimized.

Member

jirikuncar commented Apr 30, 2015

  • Please add yourself to AUTHORS.
  • Please reply if you agree to donate this (and any future) contributions to Flask-Menu package under the terms of the Revised BSD Licence.

Thank you!

@coveralls

This comment has been minimized.

coveralls commented Apr 30, 2015

Coverage Status

Coverage increased (+4.06%) to 99.44% when pulling a5bee56 on nickw444:master into 0698254 on inveniosoftware:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Apr 30, 2015

Coverage Status

Coverage increased (+4.06%) to 99.44% when pulling a5bee56 on nickw444:master into 0698254 on inveniosoftware:master.

@jirikuncar jirikuncar added this to the v0.4.0 milestone Apr 30, 2015

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Apr 30, 2015

No rush for integration, i'm just using pip at this point in time to clone my fork. The sooner the better though, keeping upstream synced isn't always fun.

I've flattened the git history and improved the commit messages (that was difficult).

Before it gets merged we should come to a decision about the following:

  • Finding a more generic name - does it have to be called classy_menu_item/register_flaskview. The implementation does depend specifically on classy and the way it formats it's endpoints, so we should think about this. In my opinion, it is appropriate, maybe just a better wording?
  • Will I globally rename flask.ext to flask_ for the entire documentation, or just keep it as it is now (where only my new section uses the flask_ convention)

Additionally;

  • Added myself to AUTHORS
  • Yes, I agree to donating these (and future) contributions to Flask-Menu under the included revised BSD license.
@coveralls

This comment has been minimized.

coveralls commented Apr 30, 2015

Coverage Status

Coverage increased (+4.06%) to 99.44% when pulling 9ffba61 on nickw444:master into 0698254 on inveniosoftware:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Apr 30, 2015

Coverage Status

Coverage increased (+4.06%) to 99.44% when pulling 9ffba61 on nickw444:master into 0698254 on inveniosoftware:master.

@jirikuncar jirikuncar self-assigned this May 1, 2015

@@ -90,6 +97,7 @@ def _active_when(self):
def register(self, endpoint, text, order=0,
endpoint_arguments_constructor=None,
expected_args=[],

This comment has been minimized.

@jirikuncar
@jirikuncar

This comment has been minimized.

Member

jirikuncar commented May 4, 2015

@nickw444 can you split your contribution to general menu improvements and new classy.py where you can introduce new decorator? You can also move there most of the documentation and reference it in docs/index.rst with .. automodule:: flask_menu.classy. What do you think?

@jirikuncar jirikuncar removed their assignment May 4, 2015

@nickw444

This comment has been minimized.

Contributor

nickw444 commented May 5, 2015

@jirikuncar, no worries, i'll implement the changes within the next few days when I get a moment.

I certainly agree with moving the new decorators for classy into a separate file.

@coveralls

This comment has been minimized.

coveralls commented May 11, 2015

Coverage Status

Coverage increased (+4.07%) to 99.45% when pulling 7a30f58 on nickw444:master into 0698254 on inveniosoftware:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented May 11, 2015

Coverage Status

Coverage increased (+4.07%) to 99.45% when pulling 7a30f58 on nickw444:master into 0698254 on inveniosoftware:master.

@nickw444

This comment has been minimized.

Contributor

nickw444 commented May 11, 2015

@jirikuncar Changes have been implemented, Also moved flask-classy to its own section in the API docs.

@nickw444

This comment has been minimized.

Contributor

nickw444 commented May 27, 2015

@jirikuncar Can you report on the status of this merge request? Thanks

@jirikuncar

This comment has been minimized.

Member

jirikuncar commented May 27, 2015

Please rebase, remove merge commit, squash related commits and amend commit messages (remove dots from first line, use line separated bullet points for commit message details - see git log - in order to help up autogenerate release notes). Thanks

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Jun 7, 2015

I have added the required changes.

Build is failing due to Travis using latest pip, and the travis.yml file using the flag --use-mirrors.

@tiborsimko

This comment has been minimized.

Member

tiborsimko commented Jun 15, 2015

Build is failing due to Travis using latest pip, and the travis.yml file using the flag --use-mirrors.

Oops, indeed. Fix in PR #34.

@jirikuncar

This comment has been minimized.

Member

jirikuncar commented Jun 16, 2015

@nickw444 you can rebase now. Please make sure that first line in commit message is max. 50 chars long and bullet points have max. 72 chars. Thanks

@register_menu(self.app, 'normal', 'Normal')
def normal():
@self.app.route('/normal/<int:id>/')
def normal(id):

This comment has been minimized.

@jirikuncar

jirikuncar Jun 16, 2015

Member

Can you please add new test instead of modifying the existing one? Thank you!

This comment has been minimized.

@nickw444

nickw444 Jul 6, 2015

Contributor

Sure, i'll get onto that this week

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Jul 21, 2015

@jirikuncar Apologies for the delay. I have added the new test as opposed to modifying the existing one. I have also changed the commit description to meet your requirements.

Cheers

@@ -90,14 +97,19 @@ def _active_when(self):
def register(self, endpoint, text, order=0,
endpoint_arguments_constructor=None,
expected_args=None,

This comment has been minimized.

@jirikuncar

jirikuncar Jul 21, 2015

Member

just for better backward compatibility, can you move it after visible_when argument?

dynamic_list_constructor=None,
active_when=None,
visible_when=None,
**kwargs):
"""Assign endpoint and display values."""
if not expected_args:

This comment has been minimized.

@jirikuncar

jirikuncar Jul 21, 2015

Member

expected_args = expected_args or []

self._endpoint = endpoint
self._text = text
self._order = order
self._expected_args = expected_args

This comment has been minimized.

@jirikuncar

jirikuncar Jul 21, 2015

Member

or even better here: self._expected_args = expected_args or []

@@ -298,6 +335,7 @@ def _register_menu_item():
endpoint,
text,
order,
expected_args=expected,

This comment has been minimized.

@jirikuncar

jirikuncar Jul 21, 2015

Member

Please move it after visible_when.

setup.py Outdated
@@ -52,7 +52,8 @@ def run_tests(self):
'pytest-cov>=1.8.0',
'pytest-pep8>=1.0.6',
'pytest>=2.6.1',
'coverage<4.0a1'
'coverage<4.0a1',
'flask-classy>=0.6.10'

This comment has been minimized.

@jirikuncar

jirikuncar Jul 21, 2015

Member

please add comma , at the end of line

)
from flask_menu.classy import (
register_flaskview,
classy_menu_item

This comment has been minimized.

@jirikuncar

jirikuncar Jul 21, 2015

Member

you can add , here too

@jirikuncar

This comment has been minimized.

Member

jirikuncar commented Jul 21, 2015

I have also changed the commit description to meet your requirements.

@nickw444 Thanks a lot for helping us to keep the commit messages consistent. Can you please finish the sentences with a dot .. Thanks again.

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Jul 21, 2015

Changes made, let me know if there's anything else.

@@ -240,6 +259,19 @@ def has_active_child(self, recursive=True):
return True
return False
def has_visible_child(self, recursive=True):
result = False

This comment has been minimized.

@jirikuncar

jirikuncar Jul 21, 2015

Member

@nickw444 can you add a short docstring here? Thanks

# -*- coding: utf-8 -*-
#
# This file is part of Flask-Menu
# Copyright (C) 2013, 2014, 2015 CERN.

This comment has been minimized.

@jirikuncar

jirikuncar Jul 21, 2015

Member

There should be only current year as it's new file.

@jirikuncar

This comment has been minimized.

Member

jirikuncar commented Jul 21, 2015

@nickw444 I have seen just two little issues in the code and an extra dot on first line in commit message.

PS: I take the last commit message problem on me as I was not very clear. We have a document about commit messages in our original repo http://invenio.readthedocs.org/en/latest/developers/git-workflow.html?highlight=git#r2-remarks-on-commit-log-messages and I see that we should extract the most important parts to CONTRIBUTING.rst in all ours repos. Thank you for your great contribution! Cheers

@nickw444

This comment has been minimized.

Contributor

nickw444 commented Jul 21, 2015

@jirikuncar - Just made those last two changes and fixed the commit message

Cheers

@jirikuncar

This comment has been minimized.

Member

jirikuncar commented Jul 22, 2015

@nickw444 everything is ok now. Can you please rebase on top of master (8769194). Thanks

@jirikuncar jirikuncar self-assigned this Jul 22, 2015

menu: Flask-Classy support and auto url_for params
* Adds classy_menu_item and register_flaskview functions/decorators.

* Flask-Classy compatibility is stored in a separate submodule: classy.py.

* Tests: Add new tests for Flask-Menu & for new classy extensions.

* Automatically get url_for parameters for url() on MenuItemMixin.
@nickw444

This comment has been minimized.

Contributor

nickw444 commented Jul 23, 2015

Rebased on top of 8769194. Cheers

@jirikuncar jirikuncar merged commit 7b704e8 into inveniosoftware:master Jul 23, 2015

1 of 2 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment