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

YAML #408

Merged
merged 8 commits into from
Nov 30, 2020
Merged

YAML #408

merged 8 commits into from
Nov 30, 2020

Conversation

PanosCodes
Copy link
Contributor

No description provided.

@PanosCodes PanosCodes changed the title WIP: YAML [WIP] YAML Sep 30, 2020
test/stdlib/YAML_test.rb Outdated Show resolved Hide resolved

def empty_marshal_data: () -> String

def load: (String) -> (Hash | String | Array)
Copy link
Member

Choose a reason for hiding this comment

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

Hash[untyped, untyped] | String | Array[untyped] ??
Or I think returning just untyped is enough.

#
# Converts the contents of the database to an in-memory Hash object, and
# returns it.
def to_hash: () -> Hash
Copy link
Member

Choose a reason for hiding this comment

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

Hash[untyped, untyped]

@soutaro
Copy link
Member

soutaro commented Oct 5, 2020

Hi!

Rewriting Hash to Hash[untyped, untyped] would make the test pass.
However, I'm not very sure if returning hash types makes much sense, because this is serialization and untyped should appear almost everywhere...

@PanosCodes
Copy link
Contributor Author

Hi!

Rewriting Hash to Hash[untyped, untyped] would make the test pass.
However, I'm not very sure if returning hash types makes much sense, because this is serialization and untyped should appear almost everywhere...

Thank you for your feedback.
So you suggest when a there are unknown types inside the hash instead of Hash[untyped, untyped] we should return untyped ?

@PanosCodes PanosCodes marked this pull request as draft October 5, 2020 06:16
@soutaro
Copy link
Member

soutaro commented Oct 5, 2020

Oh no, I'm sorry. It is about DBM. I was confusing a bit about if it was about YAML.
Hash[untyped, untyped] would make sense for method which returns a hash.

@PanosCodes
Copy link
Contributor Author

/__w/rbs/rbs/lib/rbs/errors.rb:140:in `check!': /__w/rbs/rbs/stdlib/yaml/store.rbs:32:0...53:3: Could not find super class: PStore (RBS::NoSuperclassFoundError)

The YAML Store depends on PStore for witch we don't have signature. That is probably the reason the tests fail.
Here is a PR from PStore #443

@PanosCodes
Copy link
Contributor Author

/__w/rbs/rbs/lib/rbs/errors.rb:140:in `check!': /__w/rbs/rbs/stdlib/yaml/dbm.rbs:61:2...220:5: Could not find super class: ::DBM (RBS::NoSuperclassFoundError)

YAML also depends on DBM and tries to find it.
Here is PR for that #441

@PanosCodes PanosCodes force-pushed the yaml branch 4 times, most recently from 998b56f to 3031471 Compare November 3, 2020 18:46
@PanosCodes
Copy link
Contributor Author

@soutaro Any idea why it fails to find DBM ? I believe #441 should have fixed that.

/__w/rbs/rbs/lib/rbs/errors.rb:140:in `check!': /__w/rbs/rbs/stdlib/yaml/dbm.rbs:61:2...220:5: Could not find super class: ::DBM (RBS::NoSuperclassFoundError)

@soutaro
Copy link
Member

soutaro commented Nov 14, 2020

@PanosCodes Could you rebase your branch on the latest HEAD and moving your RBS files under stdlib/yaml/0 directory?

Standard library signatures are moved in #405. (I'm sorry for breaking your work with that. 🙇‍♂️)

@PanosCodes PanosCodes force-pushed the yaml branch 2 times, most recently from bbaca49 to 8f54523 Compare November 14, 2020 12:44
@PanosCodes
Copy link
Contributor Author

@PanosCodes Could you rebase your branch on the latest HEAD and moving your RBS files under stdlib/yaml/0 directory?

Standard library signatures are moved in #405. (I'm sorry for breaking your work with that. 🙇‍♂️)

No worries 😄 But it still throws the same error 🤔

@soutaro
Copy link
Member

soutaro commented Nov 16, 2020

@PanosCodes I got it. 💪

You need to specify -r dbm -r pstore to command line to start the validation.

$ bundle exec rbs -r yaml -r dbm -r pstore validate
# You can add the following lines to Rakefile to let `rake validate` run for YAML.

if lib == ["yaml"]
  lib << "dbm"
  lib << "pstore"
end

@PanosCodes PanosCodes force-pushed the yaml branch 2 times, most recently from a83e2b8 to 539510c Compare November 16, 2020 05:40
@PanosCodes
Copy link
Contributor Author

@PanosCodes I got it. 💪

You need to specify -r dbm -r pstore to command line to start the validation.

$ bundle exec rbs -r yaml -r dbm -r pstore validate
# You can add the following lines to Rakefile to let `rake validate` run for YAML.

if lib == ["yaml"]
  lib << "dbm"
  lib << "pstore"
end

Awesome , thank you for looking into that 👍

@PanosCodes PanosCodes marked this pull request as ready for review November 25, 2020 18:31
@PanosCodes PanosCodes changed the title [WIP] YAML YAML Nov 25, 2020
@PanosCodes PanosCodes requested a review from soutaro November 25, 2020 18:31
Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

Thanks! @PanosCodes

@soutaro soutaro merged commit 44673ba into ruby:master Nov 30, 2020
@PanosCodes PanosCodes deleted the yaml branch November 30, 2020 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants