sablier-labs / v2-core

⏳ Core smart contracts of the Sablier V2 token distribution protocol
https://sablier.com
Other
288 stars 38 forks source link

Build/npm ignore file #936

Closed andreivladbrg closed 2 weeks ago

andreivladbrg commented 1 month ago

@PaulRBerg I've also included your commit

PaulRBerg commented 1 month ago

Thanks @andreivladbrg, just FYI in the future, you can also add me as a co-author.

andreivladbrg commented 1 month ago

just FYI in the future, you can also add me as a co-author.

which commit are you referring to? i used git cherry-pick and you are the author of the commit:

image

we should not use a .npmignore file because it adds very complicated logic when files are used at the same time.

sheesh, didn't know about this

Let's not merge this as is.

do you have any suggestion?

PaulRBerg commented 1 month ago

All I meant is that instead of cherrypicking, you could have added me as a co-author. Judt FYI. Cherrypicking was fine in this case.

re sheesh - just use files, as I have suggested initially. You can put negative globs like this:

{
  "files": ["!test/utils/*.t.sol"]
}

Also, why not ignore all *.t.sol files? not just those in test/utils.

andreivladbrg commented 1 month ago

just use files, as I have suggested initially. You can put negative globs like this:

@PaulRBerg I've added this in package.json but it doesn't seem to work, as seen here:

https://app.warp.dev/block/CRhLQnKpcZGWmIdLnGpveL

what if we include this CLI in the prepack:

"find . -name 'test/utils/*.t.sol' -delete",

and after we can git restore

PaulRBerg commented 1 month ago

The negative globs should definitely work - make sure to put it at the end of the array in files. And ask ChatGPT for help. It must be possible.

PaulRBerg commented 2 weeks ago

It appears that I was wrong - negative globs are not supported in the files field.

Therefore, it's impossible to ignore the *.t.sol files without using an .npmignore file.

I pushed a commit to add an .npmignore file in the test directory. It has to be added there because otherwise npm will ignore the ignore file itself.

It should be working now - please double-check before merging @andreivladbrg @smol-ninja. Then, squash and merge, please.

smol-ninja commented 2 weeks ago

Thanks @PaulRBerg for the fix. It seems to be working now: https://app.warp.dev/block/8z3gVbenfdyKFq3RsDoFKj

andreivladbrg commented 2 weeks ago

great, ty to both🚀