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

AVM: Added the TypeEnum table back to docs #4259

Closed
wants to merge 2 commits into from

Conversation

nullun
Copy link
Contributor

@nullun nullun commented Jul 13, 2022

The TypeEnum table appears to have been removed in 95c5b0e which I believe was a mistake?

@nullun nullun requested a review from jannotti July 13, 2022 14:23
@nullun nullun added the documentation Improvements or additions to documentation label Jul 13, 2022
@nullun nullun changed the title Docs: Added the TypeEnum table back AVM: Added the TypeEnum table back to docs Jul 13, 2022
barnjamin
barnjamin previously approved these changes Jul 13, 2022
@barnjamin barnjamin self-requested a review July 13, 2022 14:48
@barnjamin barnjamin dismissed their stale review July 13, 2022 14:48

thought this was docs repo

@jannotti
Copy link
Contributor

jannotti commented Jul 13, 2022

This sounds like a mistake, but can't be fixed by editing TEAL_opcodes.md directly. That file is autogenerated (that's why the codegen_verification check failed in CI).

However, I'm not positive it's a mistake. Does the table appear in the docs elsewhere? I seem to recall messing around with the table generation and thought at least one table did not belong where it was placed.

@nullun
Copy link
Contributor Author

nullun commented Jul 13, 2022

This sounds like a mistake, but can't be fixed by editing TEAL_opcodes.md directly. That file is autogenerated (that's why the codegen_verification check failed in CI).

Ah, that makes sense.

However, I'm not positive it's a mistake. Does the table appear in the docs elsewhere? I seem to recall messing around with the table generation and thought at least one table did not belong where it was placed.

I had checked TEAL_opcodes.md and couldn't find the TypeEnum table anywhere else. There is a named_integer_constants.md file that contains it, but that's only included in README_in.md and subsequently in README.md. Maybe the note for TypeEnum in the transaction field table should be changed instead?

@jannotti
Copy link
Contributor

There is a named_integer_constants.md file that contains it, but that's only included in README_in.md and subsequently in README.md. Maybe the note for TypeEnum in the transaction field table should be changed instead?

Yeah, I think that was my reasoning. It didn't seem right to have this table where it was, implying it was specific to txn. These constants can be used in other places too, like int. I'll change the note.

@nullun
Copy link
Contributor Author

nullun commented Jul 13, 2022

I made myself more familiar with how it works and it's much clearer to me now. I just add a commit with an updated note and some extra text beneath the table. But happy to close this pr if you're on it.

@jannotti
Copy link
Contributor

#4260

@jannotti jannotti closed this Jul 13, 2022
@nullun nullun deleted the docs-add-typeenum branch July 14, 2022 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants