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

[builtins/dict] add => values() method #1777

Merged
merged 4 commits into from
Jan 1, 2024
Merged

[builtins/dict] add => values() method #1777

merged 4 commits into from
Jan 1, 2024

Conversation

Melkor333
Copy link
Collaborator

C++ probably fails because I don't wrap the elements in a value.X but we'll see :)

@Melkor333
Copy link
Collaborator Author

btw the X is already missing in the doc/ref/toc-ysh.md

Copy link
Contributor

@andychu andychu left a comment

Choose a reason for hiding this comment

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

Great, thank you!

How about filling in a one line description for keys() and values() in doc/ref/chap-type-method.md ?

And perhaps 2 examples like the ones in start() end() etc.

I've been doing that lately, we are slowly creeping up :-)

dictionary = rd.PosDict()
rd.Done()

keys = [k for k in dictionary.values()] # type: List[value_t]
Copy link
Contributor

Choose a reason for hiding this comment

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

should be called values or v or vals :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

reminder :)

Oh also I think it should just be

vals = dictionary.values()

No need for list comprehension

In the keys() case we need the list comprehension, because we have to add the value.Str() wrapper

@Melkor333
Copy link
Collaborator Author

I hated the emptiness of that page so I added more examples…

Used = for the value() to show that it's not strictly strings (as it is with keys())

Copy link
Contributor

@andychu andychu left a comment

Choose a reason for hiding this comment

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

Thanks for adding the documentation! much appreciated

Add an element to a list.

var fruits = :|apple banana pear|
call fruits -> append("orange") # returns null
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to encourage a style of no space for ->, but spaces for =>` to make it look more different?

call fruits->append('orange')

vs.

var v = d => values() => reversed()

Because => can be chained and -> can't. -> generally has no return value

What do you think?

dictionary = rd.PosDict()
rd.Done()

keys = [k for k in dictionary.values()] # type: List[value_t]
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder :)

Oh also I think it should just be

vals = dictionary.values()

No need for list comprehension

In the keys() case we need the list comprehension, because we have to add the value.Str() wrapper

@andychu
Copy link
Contributor

andychu commented Dec 31, 2023

Oh just to clarify the types a bit, there are extra "wrappers" for YSH level vs. "mycpp" level:

Here's the type of a mycpp str:

s = 'foo'  # type: str

and mycpp dict:

d = {'foo': 42}   # type:  Dict[str, int]  

From core/value.asdl, here's are YSH wrappers

ysh_str = value.Str('foo')  # type: value.Str

And then a mycpp Dict with a value.Str:

var mycpp_dict = {'key': value.Str('foo')}  # type: Dict[str, value_t]

And then a YSH dict!

var ysh_dict = value.Dict(mycpp_dict)  # type: value.Dict

So yeah these extra wrappers take a bit of getting used to!

@Melkor333
Copy link
Collaborator Author

Thanks for your patience with me!
I hope the styling is now a bit better :)

@andychu andychu changed the base branch from master to soil-staging January 1, 2024 18:47
@andychu andychu merged commit 58d8470 into soil-staging Jan 1, 2024
16 of 17 checks passed
@andychu
Copy link
Contributor

andychu commented Jan 1, 2024

Thank you! I'm going to put this on #help-wanted as another great example

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