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 fixer #18

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Yaml fixer #18

wants to merge 4 commits into from

Conversation

ninsuo
Copy link

@ninsuo ninsuo commented Dec 9, 2015

I am not really satisfied by the result:

  • drops comments
  • dump's current formatting is quite not friendly (especially for hashes) (fixed)

Anyway this is a first draw, I'm open to suggestions :)

return $content;
}

$fixed = Yaml::dump($array);
Copy link
Owner

Choose a reason for hiding this comment

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

return Yaml::dump($array);

@umpirsky
Copy link
Owner

umpirsky commented Dec 9, 2015

@ninsuo Awesome work man. 👍

I agree, comments and formatting are the issue, must think about possible solutions, if you have ideas feel free to share them.

@akovalyov
Copy link
Contributor

@ninsuo I think, current impl can handle also transforming Foo\Bar to 'Foo\\Bar' with help of https://github.com/symfony/yaml/blob/master/Escaper.php. Not sure about comments, Yaml parser seems to remove it aggressively. The only way I see is to parse comments manually and add it to the result file. However, I can be blind.

@ninsuo
Copy link
Author

ninsuo commented Dec 11, 2015

@akovalyov we don't need to escape \ on unquoted/single-quoted strings.

@umpirsky
Copy link
Owner

@ninsuo I like last indention changes. 👍

@Nyholm
Copy link

Nyholm commented Dec 13, 2015

I believe @akovalyov is correct.

We know we do not edit any keys. Can we check where the comments are relative a key, store them, run Yaml parser and dump and then re-add the comments?

@ninsuo
Copy link
Author

ninsuo commented Dec 13, 2015

Yep. This may be the solution. Will try it soon 👍

@weaverryan
Copy link

There's interest about this in the Symfony core and someone asked about this on Twitter yesterday: symfony/symfony#18079. @ninsuo are you still interested in this?

Thanks!

@ninsuo
Copy link
Author

ninsuo commented Mar 10, 2016

Hello @weaverryan,

I'm quite busy those last days, if somebody from the core want to tackle this, go ahead 👍

Thank you !

A

@weaverryan
Copy link

Or some other community member ;). Thanks @ninsuo for starting this and for the quick reply - now we know this issue is open for someone else!

@umpirsky
Copy link
Owner

@ninsuo So, comments is the issue we need to solve?

@weaverryan What do you think about the formatting? Is it acceptable?

@ninsuo
Copy link
Author

ninsuo commented Mar 10, 2016

Yes, my solution is quite naive and runs Yaml parser / dumper to clean your config files up.

But using this way, you drop all comments. Bad...

I'm guessing there will be no choice but develop a Yaml fixer that will keep formatting and comments, based on regular expressions or maybe more sophisticated...

@weaverryan
Copy link

I agree with @ninsuo - this solution won't do it. But the good news is that:

  1. @ninsuo made an awesome test for this PR. If someone were able to get that test to pass with a different method, that would prove it's good.

  2. We don't have to catch all the cases. Symfony will explode if there are any parse errors, so they're easy to find by a user. It's the sheer magnitude that exist in some projects that is a problem. If this fixed 90% of use-cases, that would be just fine.

Cheers!

@umpirsky
Copy link
Owner

Just want to add that we can think bigger then this. If we can do it using some third party tool, or e.g. ruby library, I would merge that as well.

We just need yaml parser/dumper that keeps comments.

I would try to avoid using regex magic, but if someone is good with that, why not.

The point is, this is just a tool that gets he job done, it does not necessarily need to be fancy, nor perfect as @weaverryan said.

I will try to look deeper if there is something out there that we can use to easily do this.

Thank you all for the effort. 👍

@xabbuh
Copy link

xabbuh commented Mar 15, 2016

You could think about forking the Symfony YAML component. What you would need to do then is to modify it such that comments were not stripped (probably you would have to mark them somehow to be able to dump them properly again). The advantage is that the Symfony parser already knows all the deprecated features and it's easy to handle them, most of them will probably be handled implicitly (I suggest to take the one from the upcoming 3.1 release as there are some more deprecations).

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.

None yet

6 participants