-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add Mensa Mainz #110
base: master
Are you sure you want to change the base?
Add Mensa Mainz #110
Conversation
There are a build issue but not from my changes, what should i do? |
You can do nothing. ;) I haven't contributed here for a long time and don't have time to review right now. You'll have to wait until someone finds the time to review and merge, which usually is @klemens. |
No problem, I would like to fix the build problem, but at the moment I don't know enough about the project. |
As far as I can tell the problem is a 404 in the Darmstadt parser. (I'm not 100% certain, though.) That means that it is unrelated to this PR. I think it would be best if it was fixed in a separate PR anyway. ;) |
Thank you for implementing this new parser! I fixed darmstadt, but the review will have to wait until next week unfortunately. |
Thank you very much for the project, so I don't have to reinvent the wheel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only read through the code and not tested it further. It already looks really good, I especially like the comments that really help understand what's going on.
I would like @klemens to have the final say on this, but for me it looks good with minor changes.
It would be nice if you could let a formatter make the indentation consistent. The comments are not on the same level as the code and not all indentations are the same size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't run this, but I think I found a small bug. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for writing this parser! This looks quite good already. But you could use more of the build-in functionality of the LazyBuilder/OpenMensaCanteen to avoid re-implementing the same functionality. 😉
You can add your parser to build_scripts/test-urls.txt
so it gets tested in CI.
Also thanks @y0hy0h for your review!
Wow, I missed a lot of things. Especially the entry into the test urls... 😅 It's been a while... Thanks, @klemens, for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, this is basically ready to be merged apart from one minor thing!
After this is merged, we can add the parser to openmensa.org. You can do this your own account, which means you will get notifications when something fails. Alternatively, I can also add this myself. Let me know what you prefer!
Sorry for the delay, I had a lot to do in the last weeks. Now I have adapted everything according to Kalmens wishes. I can understand that you want to avoid superfluous code, I just wanted to simplify the work in the future in case you decide to add the "all" mensas option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, but I think you made a slip?
I think i can't do anything for the failed travis job. There was a problem downloading the file build_scripts/test_urls.txt, but the file is there! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I glanced over the code and it looks good to me. Only a small suggestion. :)
for canteen in canteens: | ||
parser.define(canteens[canteen], suffix='?building_id='+canteen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this alternative:
for id, name in canteens.items():
parser.define(name, suffix='?building_id='+id)
Adding the parser for the mensas of studierendenwerk-mainz.