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

fix: input name #20

Closed
wants to merge 1 commit into from
Closed

Conversation

rpalakkal
Copy link
Contributor

No description provided.

@rpalakkal rpalakkal requested a review from yi-sun January 10, 2024 23:18
@yi-sun yi-sun requested a review from ytham January 10, 2024 23:28
Copy link
Contributor

@yi-sun yi-sun left a comment

Choose a reason for hiding this comment

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

LGTM, but just want to make sure @ytham sees this to make sure there isn't any issue in another downstream dependency.

@ytham
Copy link
Collaborator

ytham commented Jan 11, 2024

@yi-sun @rpalakkal unfortunately we have inputs instead of input all the way down the stack to halo2-browser; do we want to rename them all to be consistent? (i.e. the *.circuits.ts file will use the inputs var, halo2-browser cli also uses inputs, and halo2-lib-js uses inputs as well). i'm all for doing it for consistency now versus later, but wanted to get your thoughts.

@yi-sun
Copy link
Contributor

yi-sun commented Jan 11, 2024

@ytham -- maybe we can make the CLI flags also --inputs and --outputs for consistency then?

@ytham
Copy link
Collaborator

ytham commented Jan 11, 2024

@yi-sun sure, we can do that as well; any preference on your end?

@yi-sun
Copy link
Contributor

yi-sun commented Jan 11, 2024

@ytham -- probably simpler to unify around --inputs and --outputs. The main motivation for this PR is just that right now the CLI inputs don't correctly override inputs defined in Typescript, which is blocking for some AxiomTest features

@rpalakkal rpalakkal closed this Jan 11, 2024
@rpalakkal rpalakkal deleted the fix/input-name branch January 11, 2024 02:45
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.

3 participants