feat(nodes): add ConstantNode support with Anchor parsing

[tanmay4l]

#69 cc @lorisleiva

[changeset-bot]

🦋 Changeset detected

Latest commit: 6ea901a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages

Name	Type
@codama/visitors-core	Minor
@codama/nodes-from-anchor	Minor
@codama/node-types	Minor
@codama/nodes	Minor
@codama/cli	Patch
@codama/dynamic-codecs	Patch
@codama/dynamic-parsers	Patch
@codama/renderers-core	Patch
@codama/validators	Minor
@codama/visitors	Minor
@codama/dynamic-client	Patch
@codama/errors	Minor
codama	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[lorisleiva]

Hi, thanks for the PR!

This is looking pretty good. The only thing I'm unsure of is whether on not the ConstantNode should wrap the existing ConstantValueNode which is the existing way for us to describe a type + value combo.

On one hand, this would make the ConstantValueNode the single source of truth for handling constants (visitors just need to add an implementation for this Node and it'll work everywhere in the tree).

On the other hand, this make the ConstantNode a bit awkward because it'll access data by doing something like program.constants[0].constant.value. Maybe something that can be fixed with a better naming convention?

What do you think?

[tanmay4l]

Hi, thanks for the PR!

This is looking pretty good. The only thing I'm unsure of is whether on not the ConstantNode should wrap the existing ConstantValueNode which is the existing way for us to describe a type + value combo.

On one hand, this would make the ConstantValueNode the single source of truth for handling constants (visitors just need to add an implementation for this Node and it'll work everywhere in the tree).

On the other hand, this make the ConstantNode a bit awkward because it'll access data by doing something like program.constants[0].constant.value. Maybe something that can be fixed with a better naming convention?

What do you think?

hmm interesting point about ConstantValueNode. I kept them separate because program constants need name field, which ConstantValueNode doesn't have. also constants[0].constant.value feels awkward. But I see your point about reuse.
would you prefer I refactor it to wrap ConstantValueNode? happy to change if that's the direction you want

[lorisleiva]

Hey sorry for the late reply, I was actually gonna review this properly this week. Are you still okay keeping this PR open? ☺️

[tanmay4l]

Hey sorry for the late reply, I was actually gonna review this properly this week. Are you still okay keeping this PR open? ☺️

sure haha ( ill resolve conflict )

[lorisleiva]

Thanks for this PR and again sorry for the super later review.

I've made a few comments, lmk what you think.

I also think you're missing an important test file in the packages/visitors-core/test/nodes folder (e.g. look at the packages/visitors-core/test/nodes/DefinedTypeNode.test.ts file for reference).

P.S.: Don't forget to rebase. ☺️

[alexanderguy]

Bump for this feature getting in. We'd like to use it in https://github.com/faremeter/flex . We can help with review/getting it landed, if anyone needs anything.

[tanmay4l]

Bump for this feature getting in. We'd like to use it in https://github.com/faremeter/flex . We can help with review/getting it landed, if anyone needs anything.

I apologize for the delay. I’m back now and will push the updated code soon.

[lorisleiva]

Thank for the changes! I've just got a few minor comments left but after that we should be good to merge.