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

Add missing docstrings #1882

Merged
merged 2 commits into from Nov 26, 2019
Merged

Conversation

@cburgdorf
Copy link
Contributor

cburgdorf commented Nov 25, 2019

What was wrong?

We have a bunch of types in our docs that are lacking docstrings. I believe having at least some docstring per public API is the lowest bar of requirement before we can cut a beta.

How was it fixed?

Added at least some docstring for every single public API.

Imperative present tense is being used for the docstrings.

For methods, it should rhyme with IT WILL:
Example:

def get_block:
    """
    Return the block at the tip of the chain
    """

For methods, it should rhyme with IT IS:
Example:

class SchemaAPI(ABC):
    """
    A class representing a database schema that maps values to lookup keys.
    """

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@cburgdorf cburgdorf force-pushed the cburgdorf:christoph/docs/more-api-docs2 branch from 47aa220 to aed33dc Nov 25, 2019
@cburgdorf cburgdorf force-pushed the cburgdorf:christoph/docs/more-api-docs2 branch 4 times, most recently from cbeb851 to 9955e68 Nov 25, 2019
@cburgdorf cburgdorf force-pushed the cburgdorf:christoph/docs/more-api-docs2 branch from 9955e68 to 05b8985 Nov 26, 2019
@cburgdorf cburgdorf marked this pull request as ready for review Nov 26, 2019
@@ -65,27 +65,27 @@ def __next__(self) -> int:
return STOP

def peek(self) -> int:
current_pc = self.pc
current_pc = self.program_counter

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Nov 26, 2019

Author Contributor

I have this in a separate commit but thought I could sneak it in here. I personally had to lookup what pc stands for again and I remember I already looked it up in the past. Part of this is probably just my poor memory but I also think it aligns better with the rest of the codebase to not use two letter acronyms :)

)


def validate_stack_bytes(value: bytes) -> None:
if len(value) <= 32:
return
raise ValidationError(
"Invalid Stack Item: Must be either a length 32 byte "
f"string or a 256 bit integer. Got {value!r}"
"Invalid Stack Item: Must be either a length 32 byte string. Got {value!r}"

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Nov 26, 2019

Author Contributor

Another non-doc change that I sneaked in here. I think the text was wrong.

@@ -473,7 +587,7 @@ def add_transaction(self,
def get_block_transactions(
self,
block_header: BlockHeaderAPI,
transaction_class: Type[SignedTransactionAPI]) -> Sequence[SignedTransactionAPI]:
transaction_class: Type[SignedTransactionAPI]) -> Tuple[SignedTransactionAPI, ...]:

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Nov 26, 2019

Author Contributor

Another non-doc change sneaked in here. It seems more aligned with our best practices to have the return type be the actual type that is returned (in contrast to parameter types which should accept the broadest type that the method could work with). This also aligns the method with the other methods on that class that do also use Tuple[Something, ...] as return type.

@cburgdorf cburgdorf requested a review from pipermerriam Nov 26, 2019
@@ -46,31 +46,31 @@ def __getitem__(self, i: int) -> int:

def __iter__(self) -> Iterator[int]:
# a very performance-sensitive method
pc = self.pc
pc = self.program_counter

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Nov 26, 2019

Member

I would extract this to it's own PR. 👍

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Nov 26, 2019

Author Contributor

Ooops..too late. I saw the approval and rage merged

@cburgdorf cburgdorf merged commit bc476b2 into ethereum:master Nov 26, 2019
21 checks passed
21 checks passed
ci/circleci: py36-benchmark Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py36-database Your tests passed on CircleCI!
Details
ci/circleci: py36-docs Your tests passed on CircleCI!
Details
ci/circleci: py36-lint Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-byzantium Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-constantinople Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-frontier Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-homestead Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-istanbul Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-petersburg Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-spurious_dragon Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-tangerine_whistle Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-transition Your tests passed on CircleCI!
Details
ci/circleci: py36-transactions Your tests passed on CircleCI!
Details
ci/circleci: py36-vm Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
ci/circleci: py37-database Your tests passed on CircleCI!
Details
ci/circleci: py37-lint Your tests passed on CircleCI!
Details
ci/circleci: py37-transactions Your tests passed on CircleCI!
Details
ci/circleci: py37-vm Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.