New OpenMensa model classes and use of alternative website for Aachen #70

j-maas · 2018-02-06T17:38:29Z

This PR unfortunately does two things. If you want to, I can spend the time to separate it into two distinct PRs.

OpenMensa Model

The major change concerns the addition of new model classes. The are found in the root in the openmensa_model module. They contain classes for a Canteen, Day (and DayClosed), Category, Meal, PriceWithRoles and Role, which each handle their own XML generation.

This allowed me to break down the Aachener parser into more concise functions, that each return a model object for e. g. a Meal and then aggregate them into a Category, then a Day etc.

I additionally created a model just for the Aachener parser, because the Aachener menu differs slightly form the OpenMensa model. E. g., the price is fixed for a category, not per meal. But it is straight forward to convert this Aachener model into the OpenMensa variant in the end.

I believe that this approach to generating the XML is easier to maintain than the centralized XML generator in PyOpenMensa's BaseBuilder, since it breaks down the XML into the data classes. It also helps structure the parsers' code, since it doesn't require them to input all the meals information in one single line, but encourages splitting up the parsing to the individual components.

Aachener Alternative Website

Instead of the list menu, I modified Aachen to parse from the table menu. The table is more stable, has less glitches like duplicate entries and weird formatting.

However, currently the table for the Academica does not include certain dishes, namely "Express", "Pizza Classics" and "Ofenkartoffel". This might break for some users, and thus warrants a discussion.
I've already asked the canteen's administration per mail for help with the online menu's inconsistencies, but have not received a reply, yet.

The regression tests that were in the package are now factored out of it.

klemens

This is not a complete review, just some thoughts on the model. I haven't looked at the parser.

I am not yet sure if I like the proposed model. But while I think it is good that the new model requires the explicit use of the various helpers from pyopenmensa.feed, it will currently happily generate an invalid feed.

Also this code should probably live in pyopenmensa instead.

This PR unfortunately does two things. If you want to, I can spend the time to separate it into two distinct PRs.

While that would be nice, the bigger problem is that you based your changes on your other pull request. You should rebase the changes onto master, using something like: git rebase --onto mswart/master y0hy0h/regression-test (untested)

klemens · 2018-02-06T18:13:52Z

.gitignore

@@ -1,3 +1,9 @@
 __pycache__
 build
+
+# Python Virtual Environment
+venv/


You can also add your ignores to .git/info/exclude.

Thanks, learned something very interesting!

I just tried it out, and of course it works perfectly. But I'm thinking about whether it is generally a good idea to just put them in the public .gitignore. Because there is a (slight) chance that others have similar setups and then they don't forget to add it to their .git/info/exclude and it won't get into the repo.

But if you'd prefer me to it in .git/info/exclude in this case, just tell me. ;) What about the .idea/ in there as well?

I don't use a virtual environment for this project, but if I would I wouldn't name it venv. 😉

I think only build artifacts and maybe config files that everyone has to use should be in .gitignore. Everything else should be added locally.

Yes, that sounds fair. I will do that!

klemens · 2018-02-06T18:16:37Z

openmensa_model.py

+        return '<{}: {}>'.format(self.__class__.__name__, self.__dict__)
+
+    def __eq__(self, other):
+        return isinstance(other, self.__class__) and self.__dict__ == other.__dict__


Are these function actually needed (in all classes)? You don't seem to store these objects in maps or sets.

I do store the Categorys in a Counter, which needs to know when two Categorys and consquently Meals are equal. The __repr__ was for debugging purposes.

I would have liked to use data classes, but I haven't found a way to use them without Pyhotn 3.7.

But this is specific for your model. You even use your own Aachen.Category, so this could be added there.

While I agree that it is possible to leave it out, do those functions do harm in any way? I'd even argue it's better to implement rich comparisons and representations for custom classes.

I'm honestly just not really understanding why you want to remove them. Please tell me what you think about this!

I don't think they cause any harm. They just felt redundant and distracting while I read through the model. If you want to keep them, I am also fine with that.

I totally understand, now. :) But except data classes in Python 3.7 there is no elegant solution, I fear...

I'd like the model to be easily usable by anyone, and those methods help with that, in my opinion.

They are not class specific and similar in most classes. So it might be better to implement it in a shared meta class, parent class or a decorator.

klemens · 2018-02-06T18:20:08Z

openmensa_model.py

+        return isinstance(other, self.__class__) and self.__dict__ == other.__dict__
+
+
+class DayClosed:


Does it really make sense to differentiate between a day without any meals and a closed day?

The XML has this distinction, so I did the same. I was admittedly struggling with how to model a closed day, but I prefer this explicit object over a flag. But I'm open to suggestions!

Ok, but could DayClosed (ClosedDay might be more readable) be a subtype of Day that eg raises on append (or does nothing)?

I'm finding it very difficult in Python to construct a clear hierarchy. If we let DayClosed inherit from Day, we will have to override the methods as you said. Both having it inherit and not feel not completely right to me.

My argument for separating them and not using inheritance is that I want to make their difference very explicit. (And implementation inheritance is bad.)

The more I think about your name suggestion, the more I agree. :D I will change the name to ClosedDay.

Ok, I thought about it again, and you are right, there isn't really any advantage.

I usually use statically types languages, so I automatically "think in types", but the python list doesn't really care which type it's elements have, so I guess this is fine.

klemens · 2018-02-06T18:30:30Z

openmensa_model.py

+                    price.text = price_format.format(role_price / 100)
+            else:
+                price = ET.SubElement(meal_element, 'price')
+                price.text = price_format.format(self.price / 100)


Generating prices should be part of the Price class and in it's own method, so it can be easily overwritten.

klemens · 2018-02-06T18:34:01Z

openmensa_model.py

+            if isinstance(self.price, PriceWithRoles):
+                for role in sorted(self.price.roles):
+                    price = ET.SubElement(meal_element, 'price', {'role': role.name})
+                    role_price = self.price.default + role.priceSupplement


Wouldn't it make more sense to just specify a fixed price for every role instead of splitting into base and surcharge (which are actually the correct termini). Splitting could then be implemented by extending the Price class (see next comment).

I agree. Letting the price be either an int or a dict of Prices that support formatting their own XML is a good way of handling this.

j-maas · 2018-02-07T11:51:27Z

@klemens What is an example of an invalid feed that the model will allows? I don't understand exactly what you mean.

I based this on the regression test branch because I really need such a test to make sure I don't break anything. The test has saved me from a lot of bugs already. Before merging this PR, it does make sense to merge #63. In case we decide not to merge the other one, I will rebase this so that it doesn't include or reverts those commits.

…l objects

klemens · 2018-02-07T16:56:58Z

What is an example of an invalid feed that the model will allows?

Many of the elements and attributes have various constraints, eg no empty strings allowed, role must be one of pupil, student, employee, or other, etc.

See #40 where mswart said My goal would be to adjust PyOpenMensa to prevent all invalid XML generation.

Use .git/info/exclude instead.

j-maas · 2018-02-07T18:41:39Z

Yes, I totally forgot to do such checks! I think this kind of model lends itself quite easily to do such checks. I might implement them, depending on how the model is received.

About PyOpenMensa: I looked and I could only find an incomplete API2 model. That's why I bothered creating this at all. It is meant to be used instead of the PyOpenMensa BaseBuilder and (at least conceptually) to be part of PyOpenMensa.
But PyOpenMensa is so tightly coupled to this repo that I feel it makes sense to just put the model here, since PyOpenMensa sits inside here as a submodule anyway. But I'm totally fine with moving the model to PyOpenMensa.

Maybe we can take this example as a starting point to discuss whether it makes sense to integrate PyOpenMensa into this repo.

klemens · 2018-02-07T19:13:23Z

I might implement them, depending on how the model is received.

Sounds reasonable!

I could only find an incomplete API2 model

It seems this is intended for querying the openmensa.org api. I haven't actually looked at the api, so I am not sure if we could use the same model both for creating and querying menus, but that certainly would be cool!

…whether it makes sense to integrate PyOpenMensa into this repo.

I think keeping them separate is a good idea, as one might use pyopenmensa for querying or writing a parser that is not part of this repo (e.g. because it is closed source because of an internal api).
We could also keep the api here for fast iteration and later move it to pyopenmensa.

I hope to find some time this weekend to take a closer look at the new parser.

mswart · 2018-02-07T20:44:12Z

I am open to represent the feed with explicit domain models. In some cases this might simplify feed generation and allows simpler adjustments. The current Builder interface has been extended for the years and it is probably reasonable to split its feature and work more with composition instead of inheritance.
But for many parsers a simple interface should also be available. Only calling one central addMeal methods is good enough! LazyBuilder takes care of quite a few integration and validation. No need to require to deal with a whole bunch of objects.
I propose rewriting the Builder-classes to use the model classes under the hood (or the other way around) - no need to implement two ways of generate and validate feed data.

I still thinking about the class structure itself. The proposed change reimplements most of the models in aachen.model. Especially the convert_to_openmensa_model methods looks straight to my.
Maybe a framework should support an processor interface: define classes than convert the user input like prices or notes to the wanted base format. Even the helper functions from pyopenmensa.feed might have a better implementation as class.
The LegendProcessor be constructed with the legend data and its instance is later one used to process meal names and produce notes.
At the moment it is only a vague idea but I will think about it in the next days.

Also this code should probably live in pyopenmensa instead.

Yes, the model classes should be moved to pyopenmensa. Even some classes of utils like Parser and Source are generic and should be moved.

I could only find an incomplete API2 model

It seems this is intended for querying the openmensa.org api. I haven't actually looked at the api, so I am not sure if we could use the same model both for creating and querying menus, but that certainly would be cool!

Yes, some years ago I started working on an API library integration. I think it is still a good idea! But for now I will not continue this project.
If an integration is reasonable we could make it. But without many thoughts I am against it. Generating parser feeds and receiving canteen data are very different use cases. A combination might be possible now, but on the long hand it would probably produce conflicts.
They could share a common base system but should at least provide different API entry points: like feed.Canteen and api.Canteen.
The API integration is unfinished and to my knowledge unused, so lets not invest too much work.

…whether it makes sense to integrate PyOpenMensa into this repo.

I think keeping them separate is a good idea, as one might use pyopenmensa for querying or writing a parser that is not part of this repo (e.g. because it is closed source because of an internal api).
We could also keep the api here for fast iteration and later move it to pyopenmensa.

PyOpenMensa and openmensa-parsers are very different projects and should stay that way.
PyOpenMensa is a mostly stable library with sufficient test integration and official releases. It is used by openmensa-parsers but also otherwise developed and hosted parsers.
openmensa-parsers is more adapting bunch of parsers covered with basic integration testing to be continues deployed to one specific installation.

j-maas · 2018-02-07T21:07:18Z

I forgot that there are other parsers than the one in this repo. 😅 You're both right that separating this out makes sense.

It seems this is intended for querying the openmensa.org api.

I didn't really understand what it did, and apperently jumped to conclusions about it. But this PR's kind of model is still useful, I guess.

I still thinking about the class structure itself.

Me, too. Ideally, I would like to have one model as a "single source of truth/authority". Then we could use alternative models that can be converted to this one, as I tried with the Aachener parser.

I'm still not happy with how that alternative model turned out, though. Maybe we can figure out something elegant.

PR Separation

I think it makes sense to separate this PR into one about the model and one about the Aachener website change.

I might be able to tackle this tomorrow.

j-maas · 2018-02-08T17:52:40Z

Split into #71 and #72.

Y0hy0h added 30 commits January 14, 2018 11:23

Add regression test

015a38b

Create aachen package and move parser file and tests into it

fb7c4f8

Fix aachen_test.py import and rename to regression_test.py

4615e5c

Test parsers/ on build

f909889

Make XML output deterministic

fd8547e

Base regression test on snapshots

b235233

Update .gitignore to exclude virtual environment directories

8f15ab7

Determine parser's URL programmatically

dc8cc32

Make regression tests generic

1727544

Flatten Aachener parser package

0c963c1

The regression tests that were in the package are now factored out of it.

Print feedback when updating snapshots

0172252

Add missing build dependency

65c2b26

Make regression test independent of request-mock package

d673ff6

Remove unneeded build dependency

12adcc4

Add copyable output on failed regression test

39ef9c7

Respect encoding during snapshot upgrade

626ce6b

Only allow snapshot updates for individual canteens

c8f11fc

Allow updates of all parsers-under-test' snapshots with flag

a8e1538

Use upstream PyOpenMensa determinism instead of workaround

ad40add

Detect URL's to store as snapshots automatically

9078f01

Update snapshots for Aachen

f3d1dc0

Use unittest.mock for intercepting requests

acd931e

Fix HTTP requests not being intercepted

811d3b6

Pretty print website snapshot

ff43e5d

Fix Aachener usage of requests for testability

519ac05

Update snapshots

ecce574

Refactor Aachener parser

ff3fa65

Use model for Meal

663f8eb

Use model for table entry

e7ccc7b

Add model for category

af1f8f0

Y0hy0h added 7 commits February 6, 2018 18:17

Determine available categories programmatically

8101d87

Filter out meals containing unavailability disclaimer

1df6cfe

Handle closed days

08ac1e8

Update Aachener snapshots

f524ad6

Defend against whitespace terrorism

cbb82e9

Factor Aachen into smaller methods

960463e

Refactor Aachener parser for readability

7971cae

j-maas added enhancement Parser Aachen labels Feb 6, 2018

Y0hy0h added 2 commits February 6, 2018 18:54

Export openmensa_model in setup.py

b6c05b3

Use OrderedCounter

157b9bb

klemens reviewed Feb 6, 2018

View reviewed changes

Y0hy0h added 3 commits February 7, 2018 13:18

Make OpenMensa Price closer to XML, replace it with Aachener model

74498bc

Allow complete initialization of OpenMensa model objects

2c162cc

Offload conversion from custom to OpenMensa model from parser to mode…

3fee82c

…l objects

Y0hy0h added 2 commits February 7, 2018 19:25

Move local ignored folders out of .gitignore

d92ec3d

Use .git/info/exclude instead.

Rename OpenMensa model's DayClosed to ClosedDay

8814966

This was referenced Feb 8, 2018

OpenMensa model #71

Closed

WIP: Refactor #72

Closed

j-maas closed this Feb 8, 2018

j-maas deleted the alternate-website branch March 2, 2018 22:12

j-maas restored the alternate-website branch March 2, 2018 22:12

j-maas deleted the alternate-website branch March 2, 2018 22:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New OpenMensa model classes and use of alternative website for Aachen #70

New OpenMensa model classes and use of alternative website for Aachen #70

j-maas commented Feb 6, 2018

klemens left a comment •

edited

Loading

klemens Feb 6, 2018

j-maas Feb 7, 2018

j-maas Feb 7, 2018

klemens Feb 7, 2018

j-maas Feb 7, 2018

klemens Feb 6, 2018

j-maas Feb 7, 2018

klemens Feb 7, 2018

j-maas Feb 7, 2018

klemens Feb 7, 2018

j-maas Feb 7, 2018

mswart Feb 7, 2018

klemens Feb 6, 2018

j-maas Feb 7, 2018

klemens Feb 7, 2018

j-maas Feb 7, 2018

klemens Feb 7, 2018

klemens Feb 6, 2018

klemens Feb 6, 2018

j-maas Feb 7, 2018

j-maas commented Feb 7, 2018

klemens commented Feb 7, 2018

j-maas commented Feb 7, 2018

klemens commented Feb 7, 2018

mswart commented Feb 7, 2018

j-maas commented Feb 7, 2018

j-maas commented Feb 8, 2018

		return isinstance(other, self.__class__) and self.__dict__ == other.__dict__


		class DayClosed:

New OpenMensa model classes and use of alternative website for Aachen #70

New OpenMensa model classes and use of alternative website for Aachen #70

Conversation

j-maas commented Feb 6, 2018

OpenMensa Model

Aachener Alternative Website

klemens left a comment • edited Loading

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

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

Choose a reason for hiding this comment

j-maas commented Feb 7, 2018

klemens commented Feb 7, 2018

j-maas commented Feb 7, 2018

klemens commented Feb 7, 2018

mswart commented Feb 7, 2018

j-maas commented Feb 7, 2018

PR Separation

j-maas commented Feb 8, 2018

klemens left a comment •

edited

Loading