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: handle when local dependency is already a manifest [APE-1397] #1666

Merged
merged 9 commits into from
Oct 20, 2023

Conversation

antazoey
Copy link
Member

What I did

I was annoyed when I saw ape-safe had to manually copy the manifest into the packages folder, that seemed so unnecessary.
I made it so if you use a local dependency and it is already a manifest, it just copies it in that way.

How I did it

Check if is a JSON manifest file and if it, copy it on.

How to verify it

In ape-safe, you can delete that mkdir line if you use this PR and this config:

dependencies:
  - name: safe-contracts
    local: ./safe-contracts.json
    version: 1.3.0

I can open a draft PR after this

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

@vany365 vany365 changed the title feat: local manifest deps feat: local manifest deps [APE-1397] Sep 18, 2023
@antazoey antazoey changed the title feat: local manifest deps [APE-1397] feat: handle when local dependency is already a manifest [APE-1397] Sep 18, 2023
fubuloubu
fubuloubu previously approved these changes Sep 18, 2023
Copy link
Member

@fubuloubu fubuloubu 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 side note about ape-safe, we added NPM dependencies to pivot that repo away from using the local uploaded copy eventually (so we can support more than just v1.3.0)

src/ape/api/projects.py Outdated Show resolved Hide resolved
@antazoey
Copy link
Member Author

Just a side note about ape-safe, we added NPM dependencies to pivot that repo away from using the local uploaded copy eventually (so we can support more than just v1.3.0)

Yes, I noticed the NPM thing - we can still definitely go that route. But I wish this could have been the temporary fix.

@github-actions
Copy link

This pull request is considered stale because it has been open 30 days with no activity. Remove stale label, add a comment, or make a new commit, otherwise this PR will be closed in 5 days.

@github-actions github-actions bot added the stale No activity for 30 days label Oct 19, 2023
@fubuloubu fubuloubu removed the stale No activity for 30 days label Oct 19, 2023
fubuloubu
fubuloubu previously approved these changes Oct 19, 2023
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

I'm good with this change if you want to rebase it

@antazoey antazoey merged commit e3cb2c8 into ApeWorX:main Oct 20, 2023
15 checks passed
@antazoey antazoey deleted the feat/local-manifests branch October 20, 2023 00:10
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.

2 participants