Skip to content

Conversation

@yrashk
Copy link
Contributor

@yrashk yrashk commented Jun 9, 2023

While the comparison method to be created would benefit from being able to fall back onto the default implementation, it can't do so as these default methods are private to libfyaml and are not exposed.

Solution: make them available as a public interface

While the comparison method to be created would benefit from being able
to fall back onto the default implementation, it can't do so as these
default methods are private to libfyaml and are not exposed.

Solution: make them available as a public interface
yrashk added a commit to yrashk/omnigres that referenced this pull request Jun 18, 2023
That is, it will put `t` or `f` into results. However, this is
inconsistent with the rest of the YAML file where booleans are denoted
standard YAML scalars for booleans.

Solution: use `true` and `false` values for booleans

This solution is not yet perfect because the comparison will not work
correctly for different variations of the boolean scalars in YAML.
Fixing this will require implementing custom comparator and this hinges
on being able to (ideally) reuse the standard comparison algorithms with
boolean comparison on top of it. I've previously sent a PR:
pantoniou/libfyaml#85 but it wasn't merged yet.
@yrashk
Copy link
Contributor Author

yrashk commented Jun 18, 2023

@pantoniou have you had a chance to look at this, what do you think?

@yrashk
Copy link
Contributor Author

yrashk commented Jun 18, 2023

Actually, I think there's an easier solution: just exposing int fy_token_cmp(struct fy_token *fyt1, struct fy_token *fyt2) in header files. This way, a custom comparator can use it.

I will send a separate PR.

@yrashk
Copy link
Contributor Author

yrashk commented Jun 19, 2023

I sent #87 for this and I've also discovered a bug 🐛 in fy_compare_node_user, see #86

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.

1 participant