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

Conversation

Projects
None yet
3 participants
@facelessuser
Contributor

facelessuser commented Jun 4, 2015

  • 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.
YAML now loads all strings as unicode
- 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

This comment has been minimized.

Contributor

facelessuser commented Jun 4, 2015

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

facelessuser 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.

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."""

This comment has been minimized.

@d0ugal

d0ugal Jun 4, 2015

Member

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

This comment has been minimized.

@facelessuser

facelessuser Jun 4, 2015

Contributor

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

"""Custom Loader."""
pass

This comment has been minimized.

@d0ugal

d0ugal Jun 4, 2015

Member

I'd also remove pass

This comment has been minimized.

@waylan

waylan Jun 4, 2015

Member

Actually I believe it would be an error to remove pass as nothing is defined within the class. Instead the class method add_constructor is called later (L58) which modifies the class. We can't just call add_constructor on the parent class (yaml.Loader) as that would modify the parent class (defined in the yaml lib) rather than a copy/ subclass. I agree, it is a little odd when reading the code, but the yaml lib's API requires it be that way. I suspect the comment was an attempt to clarify this (which it apparently did not do very well).

This comment has been minimized.

@d0ugal

d0ugal Jun 4, 2015

Member

Ah, so it is. I thought it was a class method below. That's a confusing API.

This comment has been minimized.

@facelessuser

facelessuser Jun 4, 2015

Contributor

I don't want to monkey patch the global loader. I don't think that is ever a good approach. Instead, I inherit from the global loader and use my own leaving the global intact in its default state. This I feel is a better approach. I can, instead of removing the comment, expound on this more in the class comment. I apologize as sometimes what is obvious in my head isn't obvious outside of it :).

This comment has been minimized.

@d0ugal

d0ugal Jun 4, 2015

Member

Sounds good. Expand on what is going on in the comment and remove the pass. We don't need a docstring and pass :)

This comment has been minimized.

@facelessuser

facelessuser Jun 4, 2015

Contributor

Ahh, then I have learned something new today :).

This comment has been minimized.

@facelessuser

facelessuser Jun 4, 2015

Contributor

Apologies if I came off sounding like you didn't know what a pass was for. I was more just trying to wrap my head around the response from my understanding albeit incorrect understanding. I've just always used the pass explicitly. It makes sense though thinking about it, as docstrings are parsed as part of the function/class and not just as a stray comment.

This comment has been minimized.

@waylan

waylan Jun 4, 2015

Member

@facelessuser perhaps the code would be clearer if the construct_yaml_str function was defined before the Loader subclass (jsut reorder them in the source). It is easy to miss that it is not indented and not a class method (especially as its first argument is self). Reordering might help eliminate that confusion for future readers.

This comment has been minimized.

@d0ugal

d0ugal Jun 4, 2015

Member

No problem, I was a bit snarky - so, sorry about that too! Reordering does seem like it would make it extra clear.

This comment has been minimized.

@facelessuser

facelessuser Jun 4, 2015

Contributor

@waylan yes, I think that is an excellent idea; much more clear.

@d0ugal

This comment has been minimized.

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

This comment has been minimized.

Contributor

facelessuser commented Jun 4, 2015

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.

Improve clarity of yaml loading function for maintenance
-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

Merge pull request #592 from facelessuser/master
YAML now loads all strings as unicode

@d0ugal d0ugal merged commit a56e03a into mkdocs:master Jun 5, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@d0ugal

This comment has been minimized.

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