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.

Sign up for GitHub

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

Jump to bottom

Reorganize dependencies #55

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Reorganize dependencies #55

wants to merge 4 commits into from

Conversation

plaintextpaco
Copy link
Collaborator

see git history for details

@plaintextpaco
manage all dependencies via npm
e3cc3ad 
we were having some compilation issues in foundry due to using an old
version of forge-std. The version in ./lib was up to date, but the one
in node_modules wasn't. I was very convinced foundry was using the files
in ./lib but that wasn't the case. Wasted like 1hr of my time.
@plaintextpaco
fix versions of packages fetched from git repos
7294eb8 
otherwise they'll clone whatever is in master and that's dangerous af
and also can cause minor build breaks. That's more likely.
@plaintextpaco plaintextpaco force-pushed the reorganize-dependencies branch 2 times, most recently from f0a5b85 to e7f0098 Compare March 1, 2023 23:08
@plaintextpaco
added a semver-range for big-name dependencies
09903b4 
rationale for this is that well established dependencies can usually be
trusted to respect semver versioning and not push breaking changes on a
patch release. smaller ones, or ones for which we're using a 0.something
or beta release, should be fixed to a particular version.

hardhat itself, however, I had to pin to keep the project working.
Later I'll remove viaIR to get a more 'normal' hardhat
@plaintextpaco plaintextpaco force-pushed the reorganize-dependencies branch from e7f0098 to 09903b4 Compare March 2, 2023 16:19
@plaintextpaco
remove imports of console.log contracts
a21d107 
this caused by a new rule in solhint which forbids it.
Which I agree with.
@plaintextpaco plaintextpaco marked this pull request as ready for review March 2, 2023 16:43
@plaintextpaco plaintextpaco requested a review from itirabasso March 2, 2023 16:44
@plaintextpaco plaintextpaco mentioned this pull request Mar 2, 2023
Draft
@itirabasso
Copy link
Collaborator

Looks good to me.

I only have one concern regarding a21d107: is it possible to exclude test contracts from that rule? I think it's actually desirable to have console.logs in tests.

@plaintextpaco
Copy link
Collaborator Author

Looks good to me.

I only have one concern regarding a21d107: is it possible to exclude test contracts from that rule? I think it's actually desirable to have console.logs in tests.

I think that's possible (eslint supports having an .eslintrc local to a directory that extends & overrides the one global to the project, it's probably the same with solhint) but I'm not sure is desireable.

Can you provide one such example on where having a console.log in a test would make it more readable? In my experience I mostly use them to get some missing info that I need in the 'figuring out'/debugging phase of development and then the commited final version can live without them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itirabasso itirabasso itirabasso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@plaintextpaco @itirabasso