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

fix: ape compile source import references #36

Merged
merged 14 commits into from
May 25, 2022
Merged

fix: ape compile source import references #36

merged 14 commits into from
May 25, 2022

Conversation

NotPeopling2day
Copy link
Contributor

@NotPeopling2day NotPeopling2day commented May 17, 2022

What I did

Added logic for fetching referenced imports

partial fix for ApeWorX/ape#623
unblocks @sabotagebeats ApeWorX/ape#81

How I did it

Read each source path and pull out imports
Add cleaned import strings to dictionary

How to verify it

Ape core implementation ApeWorX/ape#730

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)

@NotPeopling2day
Copy link
Contributor Author

Type-check currently doesn't pass because of a method I'm adding in core. I'm planning on sharing that logic between the different compilers to lookup the filename in the given paths.

ape_solidity/compiler.py Outdated Show resolved Hide resolved
Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

Test cases that would be nice:

  1. Relative imports from same directory
  2. Relative imports when going up a directory (../)
  3. Imports that are part of a remapping (?)

ape_solidity/compiler.py Outdated Show resolved Hide resolved
@antazoey
Copy link
Member

This is looking good! What is missing?

ape_solidity/compiler.py Outdated Show resolved Hide resolved
@antazoey antazoey self-requested a review May 24, 2022 15:11
tests/test_integration.py Outdated Show resolved Hide resolved
tests/test_integration.py Outdated Show resolved Hide resolved
@antazoey
Copy link
Member

def test_get_import_remapping(compiler, project, config):
    import_remapping = compiler.get_import_remapping()

    with config.using_project(project.path / "dependency"):
        # Should be different now that we have changed projects.
        second_import_remapping = compiler.get_import_remapping()

    assert import_remapping != second_import_remapping

1 similar comment
@antazoey
Copy link
Member

def test_get_import_remapping(compiler, project, config):
    import_remapping = compiler.get_import_remapping()

    with config.using_project(project.path / "dependency"):
        # Should be different now that we have changed projects.
        second_import_remapping = compiler.get_import_remapping()

    assert import_remapping != second_import_remapping

tests/test_integration.py Outdated Show resolved Hide resolved
tests/test_integration.py Outdated Show resolved Hide resolved
@NotPeopling2day NotPeopling2day marked this pull request as ready for review May 25, 2022 22:36
@NotPeopling2day NotPeopling2day merged commit f79c28a into ApeWorX:main May 25, 2022
@NotPeopling2day NotPeopling2day deleted the fix/ape-compile branch May 25, 2022 22:36
@NotPeopling2day NotPeopling2day self-assigned this May 27, 2022
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