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

feat!: sourcemap repr, str, docs and fixes #58

Merged
merged 14 commits into from
Feb 7, 2023

Conversation

antazoey
Copy link
Member

@antazoey antazoey commented Feb 2, 2023

What I did and How I Did It

My main goal here is trying to understand, so if you review this PR, please make help me do so!

So I noticed that __repr__ for SourceMap looked funny, so I starting poking in that. I found SourceMapItem and the parse() method so wanted to know more about how that worked because the docs were sort of missing.

  • Adds __repr__ implementation to SourceMap
  • Adds __str__ implementation to SourceMap
  • Adds docs to SourceMapItem
  • Renames stop to length in SourceMapItem

fixes: APE-581

How to verify it

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

ethpm_types/contract_type.py Show resolved Hide resolved
Comment on lines +177 to +178
def __str__(self) -> str:
return self.__root__
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the default __str__ method output if __repr__ is defined?

Copy link
Member Author

@antazoey antazoey Feb 2, 2023

Choose a reason for hiding this comment

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

It didn't seem like it. I added the test before I added this and it would fail at first and give me this:

SourceMap(__root__"<realsourcemap>")

Copy link
Member Author

Choose a reason for hiding this comment

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

where realsourcemap was the real source map

Copy link
Member Author

Choose a reason for hiding this comment

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

If I comment out def __str__, I get this:

assert "__root__='44...441:16;;;;;;'" == '449:4441:16:...4441:16;;;;;;'

so it is __root__='44...441:16;;;;;;'.

Does overriding __str__ here cause any issues I am not yet aware of?

@antazoey
Copy link
Member Author

antazoey commented Feb 2, 2023

This diff will be better once this merges #59
^ which contains all of the template updates

@antazoey antazoey changed the title feat: sourcemap repr and str as well as documentation improvement feat!: sourcemap repr, str, docs and fixes Feb 2, 2023
NotPeopling2day
NotPeopling2day previously approved these changes Feb 2, 2023
ethpm_types/contract_type.py Outdated Show resolved Hide resolved
ethpm_types/contract_type.py Show resolved Hide resolved
ghost
ghost previously approved these changes Feb 3, 2023
ethpm_types/contract_type.py Show resolved Hide resolved
Co-authored-by: NotPeopling2day <32708219+NotPeopling2day@users.noreply.github.com>
@antazoey antazoey dismissed stale reviews from ghost and NotPeopling2day via 80d70e1 February 3, 2023 14:09
@antazoey antazoey merged commit 5c30603 into ApeWorX:main Feb 7, 2023
@antazoey antazoey deleted the feat/sourcemap-repr branch February 7, 2023 23:38
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