Skip to content

Conversation

@tanmay4l
Copy link
Contributor

@tanmay4l tanmay4l commented Dec 12, 2025

@changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2025

🦋 Changeset detected

Latest commit: caa2700

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

This PR includes changesets to release 12 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/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

@tanmay4l tanmay4l marked this pull request as ready for review December 12, 2025 13:40
@lorisleiva
Copy link
Member

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
Copy link
Contributor Author

tanmay4l commented Jan 5, 2026

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

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