Skip to content
This repository was archived by the owner on Feb 23, 2023. It is now read-only.

Added mustGet an mustGetEntry to TypedMaped#249

Merged
evaporei merged 2 commits intographprotocol:masterfrom
streamingfast:feature/typed-map-must-get
Feb 8, 2022
Merged

Added mustGet an mustGetEntry to TypedMaped#249
evaporei merged 2 commits intographprotocol:masterfrom
streamingfast:feature/typed-map-must-get

Conversation

@maoueh
Copy link
Contributor

@maoueh maoueh commented Jan 28, 2022

This adds two small helpers that make it easier to work with JSONValue so that it's possible to get elements from objects more tersely.

Open Questions

I noticed that Entity which extends TypeMap uses get without checking null and only using typing tricks to remove the nullability that get brings.

So I'm wondering:

  • Should I remove the assert in the must... version
  • Or should we change Entity to now use mustGet instead

This adds two small helpers that make it easier to work with `JSONValue` so that it's possible to get elements from objects more tersely.

#### Open Questions

I noticed that `Entity` which extends `TypeMap` uses `get` without checking `null` and only using typing tricks to remove the nullability that `get` brings.

So I'm wondering:
- Should I remove the `assert` in the `must...` version
- Or should we change `Entity` to now use `mustGet` instead
@evaporei evaporei self-requested a review January 28, 2022 23:56
@evaporei
Copy link
Contributor

@maoueh I'm not sure I understood the type trick you've mentioned about Entity, TypedMap and get, can you explain it more, I think I'm missing something.

Besides that do you have one or a few examples JSONValue code usage? I've used it before for some tests but I've lost the code 😅 . I'm asking for this because I would like to think a bit more about how we can improve the developer experience and API design of this.

I'm not sure if I got it right but it seems that the must* methods would be more of simplifying the AS mechanisms then making our API design better, but again I didn't use it that much haha. Would love to stop a bit to think more about this 🙂

@maoueh
Copy link
Contributor Author

maoueh commented Jan 31, 2022

Right now, here the definition of Entity.getString():

  getString(key: string): string {
    return this.get(key)!.toString()
  }

This ! is the "trick" I'm talking about. This tells the compiler that this type is not null (this is something AssemblyScript is aware, that's why I though the : asserts condition I used in the other PR would work actually).

What happen at runtime, my impression is if the variable is actually null, it will create some kind of null pointer exception.

Now, about mustGet and mustGetEntry, the reasoning is mostly around reducing boilerplate for extracting fields when parsing JSON data.

Right now, one must write something like this:

const value = json.fromBytes(data)
const field1 = json.toObject().get("field1")
if (!field1) { return } 

const field2 = json.toObject().get("field2")
if (!field2) { return } 

const field3 = json.toObject().get("field3")
if (!field3) { return } 

This creates lots of boilerplate code while it could be terser. Now, this could be like that:

const value = json.fromBytes(data)
const field1 = json.toObject().get("field1")!
const field2 = json.toObject().get("field2")!
const field3 = json.toObject().get("field3")!

But this will lead to error harder to debug IMO, because the actual error will be arise when the field1 is actually used (I will validate that but I'm confident).

So I think adding mustGet and mustGetEntry make sense in this context, it would look like the code above but be more robust by reporting error immediately:

const value = json.fromBytes(data)
const field1 = json.toObject().mustGet("field1")
const field2 = json.toObject().mustGet("field2")
const field3 = json.toObject().mustGet("field3")

Here it looks like the second snippet but is more stricter but asserting that the key is there in the object.

Future

After working myself with parsing JSON, I think more quality of life is necessary to make it easier to write subgraph that deals with JSON. Indeed, the actual extraction of fields to something useful contains lot of boilerplate code. I thought a little bit about that and have some ideas.

Copy link
Contributor

@evaporei evaporei left a comment

Choose a reason for hiding this comment

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

I'm approving and I agree with your comment, the only thing is that the error message has a typo doest -> doesn't.

@maoueh
Copy link
Contributor Author

maoueh commented Feb 3, 2022

Will fix of course, let's still discuss it at the meeting soon.

@maoueh
Copy link
Contributor Author

maoueh commented Feb 7, 2022

I don't have write access to repo, so I cannot merge manually. PR is ready.

@evaporei evaporei merged commit 9a458f8 into graphprotocol:master Feb 8, 2022
@evaporei evaporei mentioned this pull request Mar 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants