Don't lose python/name tags values in mkdocs.yml #7507
Conversation
|
@pawamoy If that fix work, we owe you a beer! 🍺 |
Not in France at the moment, maybe one day when I visit friends in Alsace 😉 |
There was a problem hiding this comment.
Thanks!!! Looks like a solid solution IMO. We are leaving as-is these values between load/dump conversion without touching/analyze/execute them. So, I'm 👍 and I think it's safe to merge it.
I left some comments related to the tests and some other small things, but I think we are in a good direction here.
Also, I'd super appreciate if you can create an example in our test-builds repository (https://github.com/readthedocs/test-builds) that uses MkDocs with this scenario so I can test this locally and in production in an easy way.
@stsewd what are your thoughts here?
| """ | ||
|
|
||
| def ignore_unknown(self, node): # pylint: disable=no-self-use, unused-argument | ||
| return None | ||
|
|
||
| def construct_python_name(self, suffix, node): # pylint: disable=no-self-use, unused-argument |
There was a problem hiding this comment.
Shouldn't we need the same for others like: construct_python_module, construct_python_object and maybe others here as well?
There was a problem hiding this comment.
If you think we should immediately add support for all python/* tags, then yes. I only added logic for the python/name one since it's the only one I use, and the only one that is mentioned in the original issue #6889.
Just tell me and I'll update the PR to add support for the other tags!
There was a problem hiding this comment.
I think it depends on if it's common to have those... But I'd say it doesn't hurt to add them now
There was a problem hiding this comment.
There are 16 more Python tags:
| tag | python |
|---|---|
| !!python/none | None |
| !!python/bool | bool |
| !!python/bytes | (bytes in Python 3) |
| !!python/str | str (str in Python 3) |
| !!python/unicode | unicode (str in Python 3) |
| !!python/int | int |
| !!python/long | long (int in Python 3) |
| !!python/float | float |
| !!python/complex | complex |
| !!python/list | list |
| !!python/tuple | tuple |
| !!python/dict | dict |
| !!python/name:module.name | module.name |
| !!python/module:package.module | package.module |
| !!python/object:module.cls | module.cls instance |
| !!python/object/new:module.cls | module.cls instance |
| !!python/object/apply:module.f | value of f(...) |
Taken from https://pyyaml.org/wiki/PyYAMLDocumentation#yaml-tags-and-python-types.
Honestly I'm a bit reluctant to add all these, especially at once. Some of these tags might be more complex to support (more than one suffix/value to store in a proxy object, multiple ways to write them, etc.). I think it would be best to add them one by one as people ask for them. We now know how, so it could be quick PRs.
There was a problem hiding this comment.
I think it makes sense to add only what's required now to make MkDocs with plugins to work 👍
|
@humitos I created a branch to add a test-build: https://github.com/pawamoy/test-builds/tree/mkdocs-python-tags, but I'm not sure how to push that branch into the main repo without merging into master. |
| def yaml_dump_safely(content, stream=None): | ||
| """Uses ``SafeDumper`` dumper to write YAML contents.""" | ||
| return yaml.dump(content, stream=stream, Dumper=SafeDumper) |
There was a problem hiding this comment.
I think it may be good to have a test for this one as well to be sure that we are not transforming the YAML and it looks exactly the same after yaml_dump_safely, right?
There was a problem hiding this comment.
True! I eventually added the __eq__ method to ProxyPythonName because it made the test super easy to write 🤣
def test_yaml_dump_safely():
data = yaml_load_safely(content)
assert yaml_load_safely(yaml_dump_safely(data)) == dataexactly the same after yaml_dump_safely
It's not the case, the indentation can change (we don't have indentation here though), and an empty string is added at the end of python/name tags (see #7461 (comment)), which is correct and expected, but different from the original input.
There was a problem hiding this comment.
As far as the data is the same, I'm OK with that.
Great, thanks! I created a new branch from |
Indeed, weird, this PR does not touch JS files 😕 Maybe there was a new release of the tool? |
The problem is fixed in master, you can rebase or merge master into this branch to make the tests pass |
Co-authored-by: Manuel Kaufmann <[email protected]>
|
Squashed and rebased on master 🙂 |
|
Meh, build timeout 😢 |
|
@humitos I'm curious to know what's the usual timing between when a PR is approved and when deployed to your build system? Really eager to be able to use this fix 😃 |
|
Around one week after it's merged |
|
@humitos And how long after PR is approved would it be merged? I see some PR from last year (2019) still not merged and it concerns me... |
|
Oh, maybe they were waiting for me to rebase or something, I see there is a conflict. Let me do that. |
|
Hi @eeintech! We do our best to merge and deploy PRs. Unfortunately, our time is not enough. I'm targeting this PR to be deployed next week. Let's see if we can make it happen! |
|
@humitos I understand, thanks a lot for your hard work, I hope you can make it happen too! |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I'd still like to see this merged! I'll update each time the bot tries to stale it 😈 |
|
Not sure where we ended up with a decision about this. /cc @readthedocs/core |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@humitos Feels like this PR merge has been dragging... |
|
@eeintech just keep unstaling and I'm sure good things will happen 😁 |
There was a problem hiding this comment.
We've discussed this internally, and think this is an OK solution in the short term. In the longer term, we'd really love to not do any YAML parsing/dumping, and instead find a better way to integrate with mkdocs via an extension, or some other way that isn't so error-prone.
The main reason we are doing this currently is to add some small things to the config file. We should research and find another way of handling this, probably talking with the mkdocs team to ensure it will be supported going forward.
I'm 👍 on merging this, but we should include a # TODO: in the code linking to #7844
|
Yay! 👏 |
|
I was wondering, is this change already deployed? |
|
@steinwelberg no, sorry, we are going to migrate our platform to AWS, so deploys are frozen till that https://blog.readthedocs.com/aws-migration/ |
|
Thanks for clarifying, and no worries, as long as it will be deployed I'm fine! Now I come to wonder, The blog post mentions that you are migrating the community version to AWS. I am actually a business user. I guess this change is also going to be available for business users, right? I guess, that will also be done after the migration to AWS? hehe, lots of assumptions, I hope you can clarify 😄 . |
|
@steinwelberg we usually deploy both sites together, so .com deploys are frozen as well. And for clarification, .com is already on AWS, we are just going to migrate .org |
|
Great, I'll just have to be a bit more patient then 😉 . |
|
Thank you for the update @stsewd, I am also running into the same emoji issue, and I am looking forward to the new changes. Thank you for taking the time to look into this issue 😄 and I hope your migration goes smoothly and godspeed 💯 🎉 ! |
|
@steinwelberg @skchronicles we just deployed the new version in .org and .com, let us know if your problems are solved now. |
…s, mkdocs emoji issue fixed in PR readthedocs/readthedocs.org#7507
|
@stsewd, I can confirm that the issue was fixed and that my builds are now passing 💯 🎉 💯 🎉 💯 🎉 💯 . |
|
Thank you for your time dear maintainers ❤️ |
Close #7461