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

Specify `charset=UTF-8` when serving non-base64 files #2402

Merged
merged 3 commits into from Apr 26, 2017

Conversation

Projects
None yet
4 participants
@gnestor
Contributor

gnestor commented Apr 12, 2017

Closes #2397

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Apr 12, 2017

Member

This handles the fallback case where we don't find a mimetype for a file. Do we also want to do the same if guess_type returns text/plain as the mimetype of e.g. a .txt file?

Member

takluyver commented Apr 12, 2017

This handles the fallback case where we don't find a mimetype for a file. Do we also want to do the same if guess_type returns text/plain as the mimetype of e.g. a .txt file?

@rgbkrk

rgbkrk approved these changes Apr 12, 2017

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Apr 12, 2017

@takluyver does mimetype return the charset for a file? I wouldn't think it would, but for text/plain sending charset=utf-8 wouldn't be that bad of a guess in most cases.

When you have a .txt file and click on it in the browser, you open up an editor that correctly shows the contents as a UTF-8, it is only when you download the raw file at the moment, for example when you rename a file with UTF-8 characters within it to somefile.log for example, because that won't cause the editor to be opened when clicked.

bertjwregeer commented Apr 12, 2017

@takluyver does mimetype return the charset for a file? I wouldn't think it would, but for text/plain sending charset=utf-8 wouldn't be that bad of a guess in most cases.

When you have a .txt file and click on it in the browser, you open up an editor that correctly shows the contents as a UTF-8, it is only when you download the raw file at the moment, for example when you rename a file with UTF-8 characters within it to somefile.log for example, because that won't cause the editor to be opened when clicked.

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Apr 12, 2017

Contributor

@takluyver It sounds like a good idea to me. Does that look right?

Contributor

gnestor commented Apr 12, 2017

@takluyver It sounds like a good idea to me. Does that look right?

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Apr 13, 2017

Member

Thanks, that looks good. There's a test failure, though, because we have a test that expects text/plain:

FAIL: make sure ContentsManager returns right files (ipynb, bin, txt).
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\notebook\notebook\tests\test_files.py", line 98, in test_contents_manager
    self.assertEqual(r.headers['content-type'], 'text/plain')
AssertionError: 'text/plain; charset=UTF-8' != 'text/plain'
- text/plain; charset=UTF-8
+ text/plain

I don't think mimetype - or anything in the standard library - does encoding detection.

Member

takluyver commented Apr 13, 2017

Thanks, that looks good. There's a test failure, though, because we have a test that expects text/plain:

FAIL: make sure ContentsManager returns right files (ipynb, bin, txt).
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\notebook\notebook\tests\test_files.py", line 98, in test_contents_manager
    self.assertEqual(r.headers['content-type'], 'text/plain')
AssertionError: 'text/plain; charset=UTF-8' != 'text/plain'
- text/plain; charset=UTF-8
+ text/plain

I don't think mimetype - or anything in the standard library - does encoding detection.

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Apr 20, 2017

Contributor

Ok, tests are passing! Let's wait to see if these resolves #2397 before merging...

Contributor

gnestor commented Apr 20, 2017

Ok, tests are passing! Let's wait to see if these resolves #2397 before merging...

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Apr 26, 2017

This resolves the issue for me and log files are now downloaded correctly.

bertjwregeer commented Apr 26, 2017

This resolves the issue for me and log files are now downloaded correctly.

@rgbkrk rgbkrk merged commit 469b1c8 into jupyter:master Apr 26, 2017

4 checks passed

codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 78.21% (+0.23%) compared to d00b7e3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rgbkrk rgbkrk deleted the gnestor:utf-8 branch Apr 26, 2017

@takluyver takluyver added this to the 5.1 milestone Jul 18, 2017

@gnestor gnestor referenced this pull request Aug 3, 2017

Merged

Add 5.1.0 to changelog #2723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment