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

Enrich document with using getter function over state struct instance #15526

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

haoyang9804
Copy link
Contributor

Inspired by this issue: #15525. I guess it's better to add a document for this special return type since it's truly confusing.

Copy link

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

docs/contracts/visibility-and-getters.rst Outdated Show resolved Hide resolved
docs/contracts/visibility-and-getters.rst Outdated Show resolved Hide resolved
docs/contracts/visibility-and-getters.rst Outdated Show resolved Hide resolved
docs/contracts/visibility-and-getters.rst Outdated Show resolved Hide resolved
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Just a note on general structure here: the section on getters is already a bit too verbose IMO, with a lot of space and examples given to what are essentially obscure corner cases. I think we shouldn't make it even longer. We should consolidate the examples and make text more coherent. The focus should be kept primarily on explaining what getters are, the other things are just notes.

@haoyang9804
Copy link
Contributor Author

Just a note on general structure here: the section on getters is already a bit too verbose IMO, with a lot of space and examples given to what are essentially obscure corner cases. I think we shouldn't make it even longer. We should consolidate the examples and make text more coherent. The focus should be kept primarily on explaining what getters are, the other things are just notes.

How about merging struct example and mapping example? They are all corner cases that can lead to confusion.

@cameel
Copy link
Member

cameel commented Oct 21, 2024

Yes, they should be consolidated IMO. We could have just one example showing them all. Or even just a general description of how the ABI for a getter looks like. Which is relevant e.g. when you want to override a function with a getter and should be mentioned too.

@haoyang9804
Copy link
Contributor Author

Yes, they should be consolidated IMO. We could have just one example showing them all. Or even just a general description of how the ABI for a getter looks like. Which is relevant e.g. when you want to override a function with a getter and should be mentioned too.

Good suggestion. But I can only help with the consolidation part. I'm not very familiar with ABI.

@haoyang9804
Copy link
Contributor Author

I think the Complex example is useful for benefit users' understanding in getter function. Maybe we can remove this example and only leave examples of array and struct the mitigate confusion.

@haoyang9804
Copy link
Contributor Author

I think the Complex example is useful for benefit users' understanding in getter function. Maybe we can remove this example and only leave examples of array and struct the mitigate confusion.

I have merged the struct example and the array example. As for the Complex one that combine mapping and struct, since it implies no corner case, I removed it.
As for the ABI part, I think I can submit another pr after gaining some background.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants