Skip to content

Add support for maps#50

Merged
johnstairs merged 7 commits intomainfrom
johnstairs/maps
May 12, 2023
Merged

Add support for maps#50
johnstairs merged 7 commits intomainfrom
johnstairs/maps

Conversation

@johnstairs
Copy link
Copy Markdown
Member

@johnstairs johnstairs commented May 12, 2023

Adding a map/dictionary datatype.

The shorthand syntax is

string->int

And the full syntax (which allows more complicated value types):

!map
keys: string
values: int

They map to an std::unordered_map<K, V>, support generic type parameters, and can also be used in computed expressions (size(myMap) or myMap["myKey"]).

Addresses #47

@johnstairs johnstairs requested a review from hansenms May 12, 2023 15:30
r.map_field = {{"hello", "world"}, {"world", "bye"}};
EXPECT_EQ(r.AccessMap(), r.map_field);
EXPECT_EQ(r.AccessMapEntry(), "world");
EXPECT_EQ(r.AccessMapEntryWithComputedField(), "world");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am wondering if we should have a test that confirms that we throw if the key is not there. Right now we rely on https://en.cppreference.com/w/cpp/container/unordered_map/at to do that, so we are pretty sure that is the case, but if we later on switch the underlying implementation or we use this set of tests as a template later to write tests in another language, then we might miss it. Note that the [] operator of unordered_map creates the entry if it doesn't exist (https://en.cppreference.com/w/cpp/container/unordered_map/operator_at).

Comment thread tooling/pkg/dsl/yaml.go Outdated

func UnmarshalMapYAML(value *yaml.Node) (*GeneralizedType, error) {
if value.Kind != yaml.MappingNode {
return nil, parseError(value, "a !map must be specified with fields `keys` and optionally `values`")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think both keys and values are mandatory fields. We did discuss keys defaulting to string, but it doesn't currently right? I think that is the way it should be.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah yes, that error message is wrong. Will fix.

Copy link
Copy Markdown
Contributor

@hansenms hansenms left a comment

Choose a reason for hiding this comment

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

This looks great. Really nice with the short-hand for these too.

@johnstairs johnstairs merged commit 6c3c717 into main May 12, 2023
@johnstairs johnstairs deleted the johnstairs/maps branch May 12, 2023 20:34
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