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

[Internal, New Feature, No Breaking] Rewrite resolver for customization and user-selectable schema #172

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Paalon
Copy link
Contributor

@Paalon Paalon commented Jun 13, 2024

Rewrite Resolver. This enables to implement custom resolvers. I implemented resolutions as vectors but dicts may be better. However, it breaks the order, I leave it as it is.
There are 4 completed resolvers:

  • A resolver for the failsafe schema.
  • A resolver for the JSON schema.
  • A resolver for the Core schema.
  • A resolver for the YAML.jl v0.4.10 schema.

This PR is for #151, #152 and #170.

I implemented resolutions as vectors but dicts may be better.
However, it breaks the order, I leave it as it is.
@Paalon Paalon changed the title Rewrite resolver for customization and user-selectable schema [Internal, New Feature, No Breaking] Rewrite resolver for customization and user-selectable schema Jun 16, 2024
Copy link
Contributor

@GunnarFarneback GunnarFarneback left a comment

Choose a reason for hiding this comment

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

This is beyond my level of study of the YAML spec but the code looks fine and there's nothing I find to be strange.

@kescobo kescobo added enhancement parser internal Doesn't implicate user-facing functions labels Jun 17, 2024
@kescobo
Copy link
Collaborator

kescobo commented Jun 17, 2024

This seems like a good way to go - another option would be a NamedTuple, since we don't need it to be mutable. But this seems fine to me.

I think everything I've merged in the last couple of days are stylistic/internal cleanup, no new features, is that right? On the one hand, this doesn't implement anything user-facing, so I don't think it actually needs a feature release. On the other hand, since this is starting down the path to a major new feature, I wonder if we should get all of the minor stuff merged, cut a new minor release, and then merge this.

Another option would be to start a new release branch so that stuff could continue to get developed there, but I don't know that I can handle that level of mental overhead. What do you think @Paalon ?

@Paalon
Copy link
Contributor Author

Paalon commented Jun 18, 2024

another option would be a NamedTuple, since we don't need it to be mutable. But this seems fine to me.

If we want to change, it will be easy so think it later.

I think everything I've merged in the last couple of days are stylistic/internal cleanup, no new features, is that right? ...

Yes. This is also internal and has no external change but add new functions for further coming features. So we don't need feature release. (However we have fixed many bugs, it may be a good time to release a patch (v0.4.12))

Another option would be to start a new release branch so that stuff could continue to get developed there, ...

I want dev branch for publishing (for anyone can see and check and find a fault in my latest implementation) and experimenting the new features safely. I think there is still a serious bug around YAML document iterators etc., so master branch with only current features will need to be updated randomly in the future.

but I don't know that I can handle that level of mental overhead. What do you think ?

I need to prepare for my lab's reading seminar, so I am busy and might not be able to respond and develop until around 6 PM (GMT+9) on June 20th. It's a volunteer work, feel free to put your plans first. However the fact that there are less reviewers makes it difficult to decide the design and develop. Is it possible to give me a restricted write permission to a specific dev branch? Thankfully, @GunnarFarneback might review my work. In your free time, if you notice something wrong, then please point out and I fix it in dev branch. #172 and #173 and rewrite of src/YAML.jl and src/constructor.jl are needed for further schema development and debugging (and src/scanner.jl for YAML 1.2 & 1.1 compatibility).

@kescobo
Copy link
Collaborator

kescobo commented Jun 25, 2024

I've lost the plot - where does this stand?

Copy link

codecov bot commented Jun 29, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.09%. Comparing base (3b2b353) to head (7aa3d0c).

Files Patch % Lines
src/resolver.jl 62.50% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage   87.19%   87.09%   -0.11%     
==========================================
  Files          15       15              
  Lines        1664     1674      +10     
==========================================
+ Hits         1451     1458       +7     
- Misses        213      216       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement internal Doesn't implicate user-facing functions parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants