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

Also escape % #184

Merged
merged 2 commits into from
Feb 6, 2024
Merged

Also escape % #184

merged 2 commits into from
Feb 6, 2024

Conversation

Zinggi
Copy link
Contributor

@Zinggi Zinggi commented Feb 5, 2024

Not sure if rails uses a different yml standard, but I tried writing i18n locale files for a rails application and got this error:

/usr/local/bundle/gems/i18n-1.14.1/lib/i18n/backend/base.rb:259:in `rescue in load_yml': can not load translations from ./config/locales/de.yml: #<Psych::SyntaxError: (./config/locales/de.yml): found character that cannot start any token while scanning for the next token at line 574 column 16> (I18n::InvalidLocaleData)

For this line:

default: %d.%m.%Y

When it's quoted it works.

Not sure if it makes sense to include here and if it's actually correct. I tried scanning the standard but not really sure where to look. I found % under directives, but I don't know if it actually has a special meaning for values as well.


Requirements

  • Entry in CHANGELOG.md was created
  • Link to documentation on https://yaml.org/ is provided in the PR description
  • Functionality is covered by newly created tests

@mruoss
Copy link
Collaborator

mruoss commented Feb 5, 2024

Hi @Zinggi
Thanks for the PR. Yes, apparently % starts a directive. YamlElixir parses %d.%m.%Y correctly, though...

iex> YamlElixir.read_from_string!("default: %d.%m.%Y")
%{"default" => "%d.%m.%Y"}

Anyway, please fix the test, too. I don't see why we couldn't merge this. WDYT @jlgeering ?

@coveralls
Copy link

Pull Request Test Coverage Report for Build d5d4f5ef770f205dc6ce138d1efc6df6d19499ee-PR-184

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 08195b09fe648a028796d2f910c2ed9cff27b690: 0.0%
Covered Lines: 117
Relevant Lines: 117

💛 - Coveralls

@mruoss
Copy link
Collaborator

mruoss commented Feb 5, 2024

Oh, Simplificator is doing Elixir?! Nice!

@Zinggi
Copy link
Contributor Author

Zinggi commented Feb 6, 2024

I don't see why we couldn't merge this

Great, I'll also fix the tests then.

Here is the ruby version to compare with YamlElixir and ymlr. It uses libyaml for it's parser:

irb(main):002> YAML.load("default: %d.%m.%Y")
/home/zinggi/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/psych-5.1.2/lib/psych/parser.rb:62:in `_native_parse': (<unknown>): found character that cannot start any token while scanning for the next token at line 1 column 10 (Psych::SyntaxError)
...

irb(main):003> YAML.load("default: '%d.%m.%Y'")
=> {"default"=>"%d.%m.%Y"}

irb(main):006> YAML.dump({"default"=>"%d.%m.%Y"})
=> "---\ndefault: \"%d.%m.%Y\"\n"

Oh, Simplificator is doing Elixir?! Nice!

Yes, for around 5 years now!

@Zinggi
Copy link
Contributor Author

Zinggi commented Feb 6, 2024

The test almost looks like a copy paste error, it was the only one there without the ' around...

@jlgeering
Copy link
Collaborator

LGTM => I tried to not quote too much "for readability", i.e. there is absolutely no problem in using more quotes when it helps

@mruoss mruoss merged commit 647c8aa into ufirstgroup:main Feb 6, 2024
18 checks passed
@mruoss
Copy link
Collaborator

mruoss commented Feb 6, 2024

Thanks a lot!

@Zinggi Zinggi deleted the escape-more-characters branch February 6, 2024 08:47
@jlgeering
Copy link
Collaborator

@Zinggi we published v5.1.2 with your changes!

❤️ Thank you

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

4 participants