feature/metafeeds: Support for canteen meta feeds and index feed #2

kaifabian · 2018-10-14T01:19:51Z

This PR adds support for meta feeds (as documented in doc.openmensa.org Feed v2.1 and for a feed/canteen index (undocumented on doc.openmensa.org as of yet).

This PR creates incompatible URLs to provide a consistent naming scheme (/canteens/ for the canteen index, /canteens/<canteen>/meta for canteen metadata and /canteens/<canteen>/menu for the menu).

For both the index feed and metadata feed, a "reverse url" module (stw_potsdam/reverse.py) had to be added, that either uses a base URL derived from an environment variable (BASE_URL) or else from Flask's Request context.

(added: testing meta feed rendering in retrievaltest)

f4lco

Thorough work 👍 Change in URL structure and split into two feeds are very welcome. Will require a bit of Flask-ification :)

f4lco · 2018-10-15T11:01:34Z

stw_potsdam/feed.py

-    builder.address = canteen.street
-    builder.city = canteen.city
+
+    if skip_meta is not True:


Nitpick: if not skip_meta: ...? Anyway, why parameterise? I'd prefer speaking names such as _create_meta_builder or _create_menu_builder. Latter could possibly be inlined, because it's just the constructor call for now.

f4lco · 2018-10-15T11:03:57Z

stw_potsdam/feed.py


    for day in _active_days(menu):
        _process_day(builder, day)

    return builder.toXMLFeed()
+
+
+def render_meta(canteen, menu):


render_meta currently does not make use of (and should not require) a "menu" parameter.

f4lco · 2018-10-15T11:16:13Z

stw_potsdam/feed.py

+        (key, reverse("canteens/{}/meta".format(key))) for key in config
+    )
+
+    index_json = json.dumps(index)


In the world of Flask, this is probably

from flask import jsonify return jsonify(index)

Which adds convenience, configurability, and sensitivity to production / debug mode.
Second, I believe the method should belong to views. It is not related to feed building according to (Py)OpenMensa. It's possibly as simple as this two-line method:

# views.py @app.route('/') @app.route('/canteens') def canteen_index(): config = read_canteen_config() return jsonify({key: url_for('canteen_meta_feed', canteen_name=key, _external=True) for key in config})

Yeah, that's right.
I additionally opened mswart/pyopenmensa#16, to discuss whether this should be a feature included in pyopenmensa or not.

f4lco · 2018-10-15T11:34:43Z

stw_potsdam/reverse.py

+from flask import request
+
+
+def reverse(path):


You were looking for flask.url_for, weren't you?

I suggest to keep knowledge about the existence of Flask views (and their names, parameters, etc) fully to the views module. feed.render_meta should receive the canteen and the menu URL (or possibly a function to retrieve menu URLs). Draft:

# views.py from flask import url_for def canteen_meta_feed_xml(canteen): menu_url = url_for('canteen_menu_feed', canteen_name=canteen.key, _external=True) xml = feed.render_meta(canteen, menu_url) return _canteen_feed_xml(xml)

Basically, I was looking for flask.url_for, at least partially. I just was not sure how to 'override' this URL generation, which I want/need to use within my infrastructure, where the services are hidden behind a front-end proxy. (At least, I ran into that problem with my current implementation, before adding the BASE_URL environment variable). I will look into it. :)
At least, the urlparse.urljoin(request.url_root) foo can be replaced by a much more flask-esque flask.url_for.

OK I'm unaware of your exact requirements but here is a proposal: use config variable SERVER_NAME with the following patch:

# views.py +import os app = Flask(__name__) +app.config['SERVER_NAME'] = os.environ.get('BASE_URL')

and use BASE_URL="spam.eggs:9090" make run to execute (or similar). url_for will then be aware of host + port when constructing absolute URLs. To my belief this should somewhat closely resemble what you have been attempting.

f4lco · 2018-10-15T11:46:19Z

stw_potsdam/views.py

+
+
+def canteen_meta_feed_xml(canteen, menu):
+    xml = feed.render_meta(canteen, menu)


"menu" parameter is obsolete. Also propagates into view and test methods.

stw_potsdam/views.py

f4lco · 2018-10-15T11:56:45Z

tests/test_consistency.py

+    assert expected == actual
+
+
+def test_index_consistency():


If it's moved inside the views module, this testcase is harder to implement. I think it's not required - it is testing more of the functionality of reverse or then Flask's url_for than anything else, I believe.

kaifabian · 2018-10-15T13:15:46Z

Thanks for your comments! :)
I will look into them and update my PR accordingly (probably on wednesday).

kaifabian · 2018-10-21T19:18:51Z

Better? ;)

f4lco · 2018-10-21T19:54:09Z

stw_potsdam/feed.py

+    return builder.toXMLFeed()
+
+
+def render_meta(canteen):


Your current approach looks very neat, I'm that close to approving. Main culprit: tests do not pass due errors in argument passing. Second, some things appear half-finished to me in feed.py:

Remove unused import statement for json. (Yes, at some time I should really think about integrating basic automatic static analysis / linting)

render_menu no longer needs a canteen parameter.

render_meta still references reverse function from ex-module reverse, which rightfully ceased to exist. It should take the canteen menu URL by parameter, populated via url_for inside the viewsmodule.

I estimate a required fix time of two minutes 😉

Whoa, I see that I should not produce code when I'm in a hurry. I thought that all the tests passed (and I was surprised that everything worked out in the beginning), but it turns out that my test runner used the cached Docker image and not the current source code. 🤦‍♂️
Fixes should be up any minute.

removing debug prints that should never have been committed.

…w-potsdam-v2 into feature/metafeeds

kaifabian · 2018-10-21T22:17:57Z

Done so far. I just ran pycodestyle (former pep8) and pylint:
pep8 only complained about too long lines:

stw_potsdam\canteen.py:5:80: E501 line too long (81 > 79 characters)
stw_potsdam\canteen_api.py:9:80: E501 line too long (88 > 79 characters)
stw_potsdam\feed.py:36:80: E501 line too long (84 > 79 characters)
stw_potsdam\feed.py:37:80: E501 line too long (100 > 79 characters)
stw_potsdam\feed.py:68:80: E501 line too long (147 > 79 characters)
stw_potsdam\views.py:33:80: E501 line too long (99 > 79 characters)
stw_potsdam\views.py:34:80: E501 line too long (100 > 79 characters)
stw_potsdam\views.py:65:80: E501 line too long (90 > 79 characters)
stw_potsdam\views.py:98:80: E501 line too long (107 > 79 characters)
tests\test_retrieval.py:23:80: E501 line too long (99 > 79 characters)
tests\test_retrieval.py:29:80: E501 line too long (80 > 79 characters)

pylint (with disabled warnings missing-docstring and line-too-long) gives this:

************* Module stw_potsdam.canteen_api
C: 12, 0: Argument name "it" doesn't conform to snake_case naming style (invalid-name)
C:  5, 0: standard import "from collections import namedtuple" should be placed before "import requests" (wrong-import-order)
************* Module stw_potsdam.config
W:  7, 0: Relative import 'canteen', should be 'stw_potsdam.canteen' (relative-import)
C: 13,44: Variable name "f" doesn't conform to snake_case naming style (invalid-name)
************* Module stw_potsdam.feed
R: 36,12: Consider merging these isinstance calls to isinstance(price, (str, unicode)) (consider-merging-isinstance)
************* Module stw_potsdam.views
W:  9, 0: Relative import 'feed', should be 'stw_potsdam.feed' (relative-import)
W: 10, 0: Relative import 'config', should be 'stw_potsdam.config' (relative-import)
W: 11, 0: Relative import 'canteen_api', should be 'stw_potsdam.canteen_api' (relative-import)
C: 15, 0: Constant name "app" doesn't conform to UPPER_CASE naming style (invalid-name)
C: 19, 4: Constant name "base_url" doesn't conform to UPPER_CASE naming style (invalid-name)
C: 27, 0: Constant name "cache" doesn't conform to UPPER_CASE naming style (invalid-name)
E: 31, 4: Method 'logger' has no 'warn' member (no-member)
E: 42, 8: Method 'logger' has no 'info' member (no-member)
E: 47, 4: Method 'logger' has no 'info' member (no-member)
************* Module stw_potsdam.__main__
W:  2, 0: Relative import 'views', should be 'stw_potsdam.views' (relative-import)

Since only the base_url uppercase naming is related to this PR (and this is chosen to be consistent with the surrounding code), I think these should be addressed in another PR.
(The no-member errors are obviously false positives.)

If you'd like, I could also give those a try, but I would need to know which one you would want to be addressed, and probably you're quicker just fixing them directly. 😉

f4lco · 2018-10-22T07:12:21Z

Thanks for the info, yes, I'll postpone complaints of both tools. Those will be subject to later PRs. Are you generally interested in being a reviewer? I also noticed irregularities in the canteen API which require fixing (work in progress on my side).

kaifabian added 9 commits October 14, 2018 02:20

feature/metafeeds: Restructured API, splitted menu and meta feed

da909fc

feature/metafeeds: add field 'key' to Canteen

c526a68

feature/metafeeds: added helper function for reverse urls

12f1ae6

feature/metafeeds: added menu feed url to meta feed

380bc3e

feature/metafeeds: added missing test

24e2d6b

(added: testing meta feed rendering in retrievaltest)

feature/metafeeds: added feed index page

341dd8a

whitespace fix

7e4ceee

feature/metafeeds: added alias urls

1c4f6fc

feature/metafeeds: moved meta feed default url

69675ef

f4lco requested changes Oct 15, 2018

View reviewed changes

kaifabian and others added 5 commits October 21, 2018 19:15

feature/metafeeds: moved index generation to views

80cb737

feature/metafeeds: inlined builder instance creation

c5e6ee3

feature/metafeeds: removed unused 'menu' parameter

fc2a6f6

feature/metafeeds: replaced 'reverse' module with Flask's url_for

4055f62

Merge branch 'master' into feature/metafeeds

54fb7ca

f4lco requested changes Oct 21, 2018

View reviewed changes

kaifabian added 5 commits October 22, 2018 00:02

feature/metafeeds: removed unused import

4132b09

feature/metafeeds: removed unused function parameters

90d3faa

feature/metafeeds: moved menu feed url generation to views.py

74bf7a4

feature/metafeeds: cleanup debug prints

301853f

removing debug prints that should never have been committed.

Merge branch 'feature/metafeeds' of github.com:kaifabian/om-parser-st…

5d262aa

…w-potsdam-v2 into feature/metafeeds

f4lco merged commit c5aacfe into f4lco:master Oct 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/metafeeds: Support for canteen meta feeds and index feed #2

feature/metafeeds: Support for canteen meta feeds and index feed #2

kaifabian commented Oct 14, 2018

f4lco left a comment

f4lco Oct 15, 2018

f4lco Oct 15, 2018

f4lco Oct 15, 2018

kaifabian Oct 15, 2018

f4lco Oct 15, 2018

kaifabian Oct 15, 2018

f4lco Oct 15, 2018

f4lco Oct 15, 2018

f4lco Oct 15, 2018

kaifabian commented Oct 15, 2018

kaifabian commented Oct 21, 2018

f4lco Oct 21, 2018

kaifabian Oct 21, 2018

kaifabian commented Oct 21, 2018

f4lco commented Oct 22, 2018



		def canteen_meta_feed_xml(canteen, menu):
		xml = feed.render_meta(canteen, menu)

feature/metafeeds: Support for canteen meta feeds and index feed #2

feature/metafeeds: Support for canteen meta feeds and index feed #2

Conversation

kaifabian commented Oct 14, 2018

f4lco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaifabian commented Oct 15, 2018

kaifabian commented Oct 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaifabian commented Oct 21, 2018

f4lco commented Oct 22, 2018