Skip to content

[Docs] Fixed link to internal-function-calls #13262

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

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

xromrom
Copy link
Contributor

@xromrom xromrom commented Jul 11, 2022

Fixed link in function.rst documentation file.

@@ -79,7 +79,7 @@ Function parameters can be used as any other local variable and they can also be
parameter. This functionality is possible if you enable the ABI coder v2
by adding ``pragma abicoder v2;`` to your source file.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite outdated - abicoder v2 has been the default for a while. We should convert this note into "until version ... it was not possible to...".

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.

Thanks for the fix! The change looks good but I agree with @chriseth that it would be good to update the part about ABI encoder too while we're touching this part.

@xromrom
Copy link
Contributor Author

xromrom commented Jul 13, 2022

Hi, thanks for the review and comments!
I updated the part about "abicoder v2", @chriseth could you please look once more?

@xromrom xromrom requested a review from chriseth July 14, 2022 15:43
Until version 0.6.0 it was not possible to use a multi-dimensional array or a struct
as an input for an :ref:`external function<external-function-calls>`.
``abicoder v2`` made it possible and it's been enabled by default since version 0.8.0
(before that we had to enable it with ``pragma abicoder v2;``).
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not use we here but you instead, that's more consistent with how the document started addressing the reader, e.g. in line 58 we also use you

Other than that, LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

The coding style CI failure is due to trailing spaces in line 77 and 78

@xromrom xromrom dismissed a stale review via d86bb3e July 26, 2022 01:16
@xromrom
Copy link
Contributor Author

xromrom commented Jul 27, 2022

@Marenz thanks for the review. I changed "we" to "you" and removed trailing spaces (CI code style now is ok).
There are 2 failed CI jobs but it seems they were broken not by me.

@xromrom xromrom requested a review from Marenz July 27, 2022 02:24
Marenz
Marenz previously approved these changes Jul 27, 2022
@xromrom
Copy link
Contributor Author

xromrom commented Jul 30, 2022

@cameel, @chriseth hi!
Can we merge this PR?

@xromrom xromrom requested a review from cameel July 31, 2022 18:39
@Marenz
Copy link
Contributor

Marenz commented Aug 2, 2022

If you do a rebase on the last changes in the develop branch the tests will succeed and then we can merge it :)

@xromrom xromrom force-pushed the fixLinkToInternalFunctionCallsInDocs branch 2 times, most recently from f5e054e to 2e27d06 Compare August 9, 2022 13:29
@xromrom xromrom requested a review from Marenz August 9, 2022 14:15
@xromrom
Copy link
Contributor Author

xromrom commented Aug 9, 2022

The code was rebased.

cameel
cameel previously approved these changes Aug 10, 2022
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.

Looks good, but the review fixes should be squashed into the original commit.

I'll squash and merge.

@cameel cameel force-pushed the fixLinkToInternalFunctionCallsInDocs branch from 2e27d06 to bbf6ecf Compare August 10, 2022 14:09
@cameel cameel enabled auto-merge August 10, 2022 14:10
@cameel cameel merged commit 3c0a735 into ethereum:develop Aug 10, 2022
@xromrom xromrom deleted the fixLinkToInternalFunctionCallsInDocs branch August 11, 2022 01:09
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