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 constructor part 1 #173

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

Conversation

Paalon
Copy link
Contributor

@Paalon Paalon commented Jun 13, 2024

Abstract

  • This is no breaking PR.
  • This adds new features.
  • This is internal change.
  • This is required to develop other features.

Detail

Rewrite construct functions in src/constructor.jl. Add constructor functions for the three essential YAML schemata. However this is not completed because processing for tag !!merge and !!value are still tightly connected to construct_mapping etc.

  • Organize constructors of ConstructorError.
  • Add TODO comments.
  • Change function flatten_mapping to always return nothing.
  • The following are the list of renamed and relocated functions from src/constructor.jl to src/constructor_yaml_jl_0_4_10.jl for the YAML.jl 0.4.10 schema:
    • construct_yaml_nullconstruct_yaml_jl_0_4_10_schema_null
    • bool_valuesyaml_jl_0_4_10_schema_bool_values
    • construct_yaml_boolconstruct_yaml_jl_0_4_10_schema_bool
    • construct_yaml_intconstruct_yaml_jl_0_4_10_schema_int
    • construct_yaml_floatconstruct_yaml_jl_0_4_10_schema_float
    • timestamp_patyaml_jl_0_4_10_schema_timestamp_regex
    • construct_yaml_timestampconstruct_yaml_jl_0_4_10_schema_timestamp
    • construct_yaml_omapconstruct_yaml_jl_0_4_10_schema_timestamp
    • construct_yaml_pairsconstruct_yaml_jl_0_4_10_schema_timestamp
    • construct_yaml_setconstruct_yaml_jl_0_4_10_schema_set
    • construct_yaml_strconstruct_yaml_jl_0_4_10_schema_str
    • construct_yaml_seqconstruct_yaml_jl_0_4_10_schema_seq
    • construct_yaml_mapconstruct_yaml_jl_0_4_10_schema_map
    • construct_yaml_objectconstruct_yaml_jl_0_4_10_schema_object
    • construct_yaml_binaryconstruct_yaml_jl_0_4_10_schema_binary
    • construct_undefinedconstruct_undefined_yaml_jl_0_4_10_schema: this requires test's modification.
    • default_yaml_constructorsyaml_jl_0_4_10_schema_constructors: this requires test's modification.
  • Similar functions are defined for the failsafe, the JSON and the Core schemata in src/constructor_failsafe.jl, src/constructor_json.jl and src/constructor_core.jl.
  • Add tests for the four schemata.

This PR does not change any commonly used API (load, load_all, write, etc.).

Paalon added 10 commits June 13, 2024 19:18
Define constructors for each schema. However, it is not completed. So
this is the part 1.
Because the two tags: `!!merge` and `!!value` remain
in the construction of mapping.
I added a bit modification in the constructor of `ConstructorError`
to increase the readability.
And also I added some TODO comments and my thoughts.
@Paalon Paalon changed the title Rewrite constructor part 1 [Internal, New Feature, No Breaking] Rewrite constructor part 1 Jun 16, 2024
@kescobo
Copy link
Collaborator

kescobo commented Jun 25, 2024

Is this still live?

function add_multi_constructor!(func::Function, constructor::Constructor, tag::Union{String, Nothing})
constructor.yaml_multi_constructors[tag] = func
constructor
end

Constructor(::Nothing) = Constructor(Dict{String,Function}())
SafeConstructor(constructors::Dict = Dict(), multi_constructors::Dict = Dict()) = Constructor(merge(copy(default_yaml_constructors), constructors), multi_constructors)
# Paalon: I don't know what is safe.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe unsafe refers to things like this that you can do in pyyaml:

$ python
Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import yaml
>>> yaml.load("""!!python/object/apply:subprocess.Popen\n- [echo, hello]\n""", Loader=yaml.Loader)
<Popen: returncode: None args: ['echo', 'hello']>
>>> hello

@kescobo kescobo added the internal Doesn't implicate user-facing functions label Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Doesn't implicate user-facing functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants