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

Add ignore_serialize for YAML::Serializable #13556

Merged
merged 9 commits into from
Jun 18, 2023

Conversation

meatball133
Copy link
Contributor

resolves #13345

Based on the pr of #11804, and should as that pr not include any breaking changes.

@beta-ziliani
Copy link
Member

Thanks @meatball133! Overall it looks good, but the tests are failing. I haven't investigated the reasons, but it seems only the new tests are affected.

@meatball133
Copy link
Contributor Author

It is one test case there should be one extra space. I did some tests with my new locally compiled Crystal, and it didn't have that space. But on the online test, it does. I will add the space in any case and hopefully shouldn't fail.

@meatball133
Copy link
Contributor Author

Alright seems like different operating system wants different results.

spec/std/yaml/serializable_spec.cr Outdated Show resolved Hide resolved
spec/std/yaml/serializable_spec.cr Outdated Show resolved Hide resolved
spec/std/yaml/serializable_spec.cr Outdated Show resolved Hide resolved
@HertzDevil
Copy link
Contributor

HertzDevil commented Jun 15, 2023

The trailing whitespace is tied to the libyaml version. It is fixed in 0.2.5: yaml/libyaml#186

Reproduction:

require "yaml"

class Foo
  include YAML::Serializable

  @[YAML::Field(emit_null: true)]
  getter x : String?

  def initialize
  end
end

# "---\nx: \n" before 0.2.5
Foo.new.to_yaml # => "---\nx:\n"

@meatball133
Copy link
Contributor Author

So I don't understand, do I have to do something or will this issue automatically fix itself?

@HertzDevil
Copy link
Contributor

The specs must either strip all whitespace at the end of each line or use different expectations depending on YAML.libyaml_version. There are a few examples of the latter in the spec suite

Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@straight-shoota straight-shoota added this to the 1.9.0 milestone Jun 15, 2023
@straight-shoota straight-shoota changed the title Add ignore_serialize for YAML::Serializable Add ignore_serialize for YAML::Serializable Jun 18, 2023
@straight-shoota straight-shoota merged commit 82673dd into crystal-lang:master Jun 18, 2023
50 checks passed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extended ignore_serialize for YAML::Field
6 participants