fix: block subdirectories on realm packages #2155
Conversation
Villaquiranm
requested review from
jaekwon,
moul,
piux2 and
deelawn
as code owners
| if isRealm && file.IsDir() { | ||
| // currently realm packages does not support subpackages https://github.com/gnolang/gno/issues/1041 | ||
| panic("not supported yet") | ||
| } | ||
| if file.IsDir() || |
There was a problem hiding this comment.
If you look at this check, we are already ignoring subdirectories.
I don't think panicking is a good idea.
There are many reasons why I want to have subdirectories in realms:
-
Nested Independent Realms:
Imagine I have two realms:
examples/gno.land/p/demo/fooandexamples/gno.land/p/demo/foo/bar.
Right now, I can publish both realms in two transactions. However, with these changes, I won't be able to publishfoosince it containsbar. -
Local Directories:
Imagine I have directories like
/testsor/docsthat I don't want to publish but want to keep locally. I need to remove them to be able to publish realm.
There was a problem hiding this comment.
Hello @harry-hov thanks for your review 👍
I don't think panicking is a good idea.
I think that is just what the issue ask us to do (maybe it not relevant anymore ?)
There are many reasons why I want to have subdirectories in realms:
Nested Independent Realms:
Imagine I have two realms: examples/gno.land/p/demo/foo and examples/gno.land/p/demo/foo/bar.
Right now, I can publish both realms in two transactions. However, with these changes, I won't be able to publish foo since it contains bar.
This should apply only for r Realms not for p Realms so you will be able to keep using the subdirectory on examples/gno.land/p/demo/foo and examples/gno.land/p/demo/foo/bar
Local Directories:
Imagine I have directories like /tests or /docs that I don't want to publish but want to keep locally. I need to remove them to be able to publish realm.
I think you have a really good point here, maybe we will need to only block the subdirectories that contains any of *.gno, gno.mod files ?
There was a problem hiding this comment.
@moul @zivkovicmilos WDYT about the panic comment? The initial issue has panic in the scope.
closes #1041
Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description