-
Notifications
You must be signed in to change notification settings - Fork 11
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 dead simple API #34
Conversation
Hmmm 🤔 |
Nechápu? Travis řve nesmysly. Oddebuguju jindy. |
pyvecorg/views.py
Outdated
@app.route('/favicon.ico') | ||
def favicon(): | ||
static = os.path.join(app.root_path, 'static') | ||
return send_from_directory(static, 'favicon.ico', | ||
mimetype='image/vnd.microsoft.icon') | ||
return send_from_directory(static, 'favicon.ico', mimetype='image/x-icon') |
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.
According to the Internet Assigned Numbers Authority (IANA), all .ico file falls under the MIME type image/vnd.microsoft.icon which is what Travis tells you anyway.
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.
Oh! I understood the error message the other way around 🤦♂️ Thanks for the help!
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.
(my comment in 029dad2 being a proof 😄 )
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'd appreciate a clickable link in footer. Otherwise (except the icon mime type), it looks good.
So locally I get following with the new setup:
The previous setup passed fine locally, but blew up on CI. So is this Linux vs macOS thing?! |
So now it passes on CI, but it probably isn't portable to macOS ¯\_(ツ)_/¯ Not such an issue as it all works with Ready for a re-review. Feel free to merge if it feels ok. |
It could also be a difference in Python or Flask versions. Elsa is quite picky here (for good reasons actually, but sometimes unfortunately things break.) |
pyvecorg/views.py
Outdated
|
||
@app.route('/api.json') | ||
def api_redirect(): | ||
return redirect(url_for('api', lang=detect_lang(request))) |
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.
This doesn't actually work for a static page – the language is selected at build time.
You're aware of that, right?
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.
Good catch. :)
Could you rebase the mime change there and back not to be here? |
lang detection won't work for a static site - #34 (comment) let's assume most visitors understand Czech
fe790aa
to
427b940
Compare
Příprava na pyvec/docs.pyvec.org#11 (comment)