Skip to content
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

YAML now loads all strings as unicode #592

Merged
merged 2 commits into from
Jun 5, 2015
Merged

YAML now loads all strings as unicode #592

merged 2 commits into from
Jun 5, 2015

Conversation

facelessuser
Copy link
Contributor

  • All strings are redirected to be loaded as unicode
  • I was experiencing an issue on my system where I was getting errors
    trying to access config file objects because they were not being closed.
    In all cases, once the YAML content was loaded, we were done accessing
    the file content. YAML object will close file access in the objects
    once processing the content is complete.

- All strings are redirected to be loaded as unicode
- I was experiencing an issue on my system where I was getting errors
trying to access config file objects because they were not being closed.
In all cases, once the YAML content was loaded, we were done accessing
the file content.  YAML object will close file access in the objects
once processing the content is complete.
@facelessuser
Copy link
Contributor Author

Looks like everything checked out in CI. I probably won't be able to address any issues until late tonight my time, but I think this is a pretty straight forward pull.

@waylan
Copy link
Member

waylan commented Jun 4, 2015

As an interesting test of this, I would like to see if the tests still pass with the changes to mkdocs/config/config_options.py in 6bb45cb reverted. I think the tests added in that commit should probably remain--only remove the str checks.

Also, this PR should probably have a note added to the release notes.

One other potential concern: what happens when a user passes a value in on the command line? Specifically, is it Unicode? I realize only a few of the config settings are even available from the command line, but it might be worth looking into. I have not checked this myself.

@facelessuser
Copy link
Contributor Author

As an interesting test of this, I would like to see if the tests still pass with the changes to mkdocs/config/config_options.py in 6bb45cb reverted. I think the tests added in that commit should probably remain--only remove the str checks.

Yes removing str allows it to still pass the test suites with this pull.

Also, this PR should probably have a note added to the release notes.

I can do that later today.

One other potential concern: what happens when a user passes a value in on the command line? Specifically, is it Unicode? I realize only a few of the config settings are even available from the command line, but it might be worth looking into. I have not checked this myself.

I have not checked this myself either. Though that is a general Unicode support thing. This issue is specifically just enforcing an all Unicode yaml load, not to bring mkdocs up to a 100% Unicode safe state. I think eventually an in-depth analysis of Unicode handling should be performed, but this is probably not the place for that. What happens after the load needs to be looked at separately, but it is definitely something important to look into, especially as we are now enforcing unicode_literals everywhere via #585. One thing is for sure, if there are holes in Unicode support, they will eventually pop up in the issue tracker :).


class Loader(loader):

"""Custom Loader."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment can be removed, it doesn't tell me anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'm just in a habit now of providing comments for classes and methods, I kill it in the next rev.

@d0ugal
Copy link
Member

d0ugal commented Jun 4, 2015

Other than the comment update and a quick addition to the release notes, this looks great. Thanks very much! I'll merge it right after you update.

I'd like to remove the finally that closes files, but I'll do that in a follow up. It should be the responsibility of the tests to clean up.

Thanks!

@d0ugal d0ugal added this to the 0.14.0 milestone Jun 4, 2015
@facelessuser
Copy link
Contributor Author

I'd like to remove the finally that closes files, but I'll do that in a follow up. It should be the responsibility of the tests to clean up.

Yeah, my intentions wasn't to have it there at all, but I realized I would have to massage some other things outside the scope of this pull to have it not there, and I was trying to get this in today as I knew you were prepping for a bug fix, major release?

I will wrap this up tonight.

-Reorganize the yaml loading function to make it more clear what is
happening.
-Comment more to to make explicitly clear what is happening.
-Remove unnecessary 'pass'
-Make a note that 'finally' should be removed when the root of the issue
is cleared up.
-Update release notes with info on change.
d0ugal added a commit that referenced this pull request Jun 5, 2015
YAML now loads all strings as unicode
@d0ugal d0ugal merged commit a56e03a into mkdocs:master Jun 5, 2015
@d0ugal
Copy link
Member

d0ugal commented Jun 5, 2015

Great! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants