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

Errors when spaces are present before/after bool values in the yaml files. #19

Merged
merged 5 commits into from
Sep 15, 2024

Conversation

BitlyTwiser
Copy link
Contributor

Good day! I most enjoy the works you humans have curated for yaml parsing in Zig! I was glad to stumble upon your package the other day, which, so far, is working great for my needs. 😄

However, one thing I ran into was the following error when spaces were present after bool values in the yaml:
Error:
Screenshot from 2024-09-14 14-19-52

Yaml with a space in it:
Screenshot from 2024-09-14 14-20-07

Now, it may be rare that this actually appears in the wild, but in my case I ran into it quick as my editor was adding spaces! Thus, I am be a fool, but it was fun to track down to discern where the error was coming from in your code.

So I wanted to make a PR to do the following:

  1. Fix the error and trim the spaces from the presented value when a bool or string expression.
  2. Add some tests that included bools
  3. Add another test for displaying the use case of multiple values in a yaml file (since this is what I am using this for in a side project)

Quick screen shot of all tests passing as well:
Screenshot from 2024-09-14 14-17-07

I bid you a grand day, let me know if anything is off here, always glad to adjust and accept any and all feedback! 🙇

…er value to avoid errors. Adjusted and added tests
@pwbh
Copy link
Owner

pwbh commented Sep 15, 2024

Hey and thanks for using Ymlz and your contribution! It's looking good to me, only thing I'd like is to have a standalone yml file for this specific test, because 98YD.yml is a test case form the yaml-test-suite and I don't want to bloat it. You can just copy over what you've added + 98YD.yml content to a new file with a title for this specific test case and I'll happily approve your PR 😃.

…Moving yaml tests to more canonical locations on disk. Adding floats in test
@BitlyTwiser
Copy link
Contributor Author

Huzzah! Thank you @pwbh for your time and review! 😄

I made a few adjustments as per your desires to avoid bloating 98YD (quite nonsensical there, I should have thought of such things initially! 🙇 )

I also added in the trim to int's as I noticed that would also cause the same issue as with bools. I added some floats/int's into the tests as well to ensure everything there remained functional. 🙏

Let me know if all looks well there, I appreciate your work and time.

@pwbh
Copy link
Owner

pwbh commented Sep 15, 2024

Yep looking great! I am a bit curious now though, if all base functions now trimming their value, I wonder if we could do something like this in getExpressionValue

fn getExpressionValue(self: *Self, expression: Expression) []const u8 {
    _ = self;

    const value = switch (expression.value) {
        .Simple => expression.value.Simple,
        .KV => expression.value.KV.value,
        else => @panic("Not implemeted for " ++ @typeName(@TypeOf(expression.value))),
    };

    return std.mem.trim(u8, value, " ");
}

and call it a day :), could you test it and see if it works, and if so tweak this PR? If its not working as I am imagining in my head let me know, I'll approve this version and merge, thanks!

…reated helper function to trim values at a higher level. Adjusted tests to specifically test spaces
@BitlyTwiser
Copy link
Contributor Author

BitlyTwiser commented Sep 15, 2024

@pwbh marvelous thought indeed! In fact, I attempted to implement this precise thing whilst adding the changes (as I noticed the same pattern of use here, adding the trim in multiple places was growing stale)

However, it seems that this will break the multiline parsing:
Screenshot from 2024-09-15 09-30-17

Since parseSimpleExpression is called in multiple area's, it seems trimming the spaces for all executions causes some issues with parseMultilineString.

To combat this, I made the helper function: getExpressionValueWithTrim to be called in all other cases (simple string, bool, int) as to trim the value and return what we desire. 😃

I think this cleans it up a smidge, avoiding the multiple calls to trim all over, which was growing cumbersome! 😅

Let me know if these adjustments work for you!

src/root.zig Outdated
Comment on lines 424 to 428
return std.mem.trim(u8, switch (expression.value) {
.Simple => expression.value.Simple,
.KV => expression.value.KV.value,
else => @panic("Not implemeted for " ++ @typeName(@TypeOf(expression.value))),
}, " ");
Copy link
Owner

Choose a reason for hiding this comment

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

Yep, this works for me, but I think something like this would be a bit cleaner 🫶

Suggested change
return std.mem.trim(u8, switch (expression.value) {
.Simple => expression.value.Simple,
.KV => expression.value.KV.value,
else => @panic("Not implemeted for " ++ @typeName(@TypeOf(expression.value))),
}, " ");
const value = self.getExpressionValue(expression);
return std.mem.eql(u8, value, " ");

Copy link
Owner

Choose a reason for hiding this comment

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

@BitlyTwiser just this small comment and we are ready to merge!

Copy link
Contributor Author

@BitlyTwiser BitlyTwiser Sep 15, 2024

Choose a reason for hiding this comment

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

Certainly! I do enjoy the usage of the getExpressionValue here, I shall update as we speak!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huzzah! I made the following adjustments per your feedback:

        fn getExpressionValueWithTrim(self: *Self, expression: Expression) []const u8 {
            return std.mem.trim(u8, self.getExpressionValue(expression), " ");
        }

Let me know if that seems nonsensical and I can adjust as needed 😄

Copy link
Owner

Choose a reason for hiding this comment

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

Looks great, thanks again!

@pwbh pwbh merged commit ff146e2 into pwbh:master Sep 15, 2024
1 check passed
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.

2 participants