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

Allow setting the newline character when serializing #747

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

polterguy
Copy link
Contributor

Welcome!

Thanks for your interest in contributing to this project. Any contribution will
be gladly accepted, provided that they are generally useful and follow the
conventions of the project.

  1. Please create one pull request for each feature. This results in smaller pull requests that are easier to review and validate.

  2. Avoid reformatting existing code unless you are making other changes to it.

    • Cleaning-up of usings is acceptable, if you made other changes to that file.
    • If you believe that some code is badly formatted and needs fixing, isolate that change in a separate pull request.
  3. Always add one or more unit tests that prove that the feature / fix you are submitting is working correctly.

  4. Please describe the motivation behind the pull request. Explain what was the problem / requirement. Unless the implementation is self-explanatory, also describe the solution.

    • Of course, there's no need to be too verbose. Usually one or two lines will be enough.
  5. Follow the project's coding conventions

@EdwardCooke
Copy link
Collaborator

Only one comment on your PR, once that's fixed, I'll merge it in and we can get this closed.

@polterguy
Copy link
Contributor Author

Great, where can I find this comment ...?

@@ -122,6 +128,19 @@ public EmitterSettings WithMaxSimpleKeyLength(int maxSimpleKeyLength)
);
}

public EmitterSettings WithNewLine(string newLine)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to include NewLine in the other With methods, otherwise, it'll get lost if another With method is called after the WithNewLine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, great - I'll figure it out - Thx

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be pretty simple, it looks like that same thing was missed for the indented sequences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, as I went through the file, I saw that indentSequences was suffering from the same problem, as in if invoking any other "With" method after having applied indentSequences, it would discard the previously applied value. I'm not sure how to isolate this in a separate pull request, since it's a CTOR invocation, and the newLine value is after the indentSequences value in the CTOR arguments. Anyways, I've pushed now, including the fix for indentSequences ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing the indent at the same time. Looks like the Canonical method was missed though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is one method, but it only allows for setting it to true. will you merge this in anyways? I guess it's an unrelated issue ...?

Screenshot 2022-12-01 at 8 20 15 AM

If you're merging, when can we have a new release? I need the NuGet package in our own stuff ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I cut releases right after merging in. You should see a new package up on nuget in a few minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx Edward, you're a champ ^_^

@EdwardCooke
Copy link
Collaborator

I just re-commented it. It said it was pending, whatever that means.

@EdwardCooke EdwardCooke changed the title Fix for #746 Allow setting the newline character when serializing Dec 1, 2022
To avoid loosing information when another "With" method is invoked afterwards
@EdwardCooke EdwardCooke merged commit 48466e7 into aaubry:master Dec 1, 2022
@EdwardCooke
Copy link
Collaborator

Looks like the github action that cuts the release is failing, I'll have to look at it tomorrow or this weekend, most likely the weekend. I suspect we need to upgrade the version of dot net that the build project uses since it's targeting 3.1 and the github agents apparently only have 6.0 and 7.0 on them.

https://github.com/aaubry/YamlDotNet/actions/runs/3590103979/jobs/6043184163#step:4:24

Once I get it resolved, I'll post back here.

@polterguy
Copy link
Contributor Author

Thx

@EdwardCooke
Copy link
Collaborator

This is on it's way out to NuGet. Should be there in a few minutes.

@polterguy
Copy link
Contributor Author

Thx mate

@polterguy
Copy link
Contributor Author

Actually, I can't find it? Not sure why. The last version is 12.0.2 that was published 2 months ago ...

@polterguy
Copy link
Contributor Author

@aaubry - Hi Antoine, not sure why, but there's still no updated version at NuGet ...?

@aaubry
Copy link
Owner

aaubry commented Dec 5, 2022

This feature has been released in version 12.1.0.

@polterguy
Copy link
Contributor Author

Thx, finally I can see it at NuGet :)

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.

None yet

3 participants