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

Remove duplicated random valid namespace #838

Merged
merged 4 commits into from
Oct 7, 2022
Merged

Remove duplicated random valid namespace #838

merged 4 commits into from
Oct 7, 2022

Conversation

raphaelts3
Copy link
Contributor

Closes #836

Hey, @rootulp I'm not an expert go, so I don't know if the move I've made is the best possible, but it did its job. That said if you have any suggestions to improve that I'm open to adjusting it.

PS: Since the repo doesn't contain the tag hacktoberfest, would be possible to label this PR with hacktoberfest-accepted so it can be validated there by the rule below?

PR/MRs that also have the “hacktoberfest-accepted” label cannot be marked as spammy via a label.

@rach-id rach-id requested a review from rootulp October 5, 2022 17:26
rach-id
rach-id reviewed Oct 5, 2022
View reviewed changes
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

Thanks a lot for helping out with this. LGTM. Left a blocking question

pkg/utils/util.go Outdated Show resolved Hide resolved
pkg/utils/util.go Outdated Show resolved Hide resolved
rootulp
rootulp previously approved these changes Oct 5, 2022
View reviewed changes
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @raphaelts3 ! Excited to hear you're participating in hacktoberfest.

I think the label you mentioned is sufficient to pass their rule based on:
Screen Shot 2022-10-05 at 2 36 16 PM

pkg/utils/util.go Outdated Show resolved Hide resolved
pkg/utils/util.go Outdated Show resolved Hide resolved
pkg/utils/util.go Outdated
tmrand "github.com/tendermint/tendermint/libs/rand"
)

func RandomValidNamespace() namespace.ID {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional]

Suggested change
func RandomValidNamespace() namespace.ID {
func RandomMessageNamespace() namespace.ID {

Note: accepting this suggestion implies renaming usage across all files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepted and renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new name still doesn't feel like the best one but seems less troublesome than the previous one.

Thanks for this article, very good points there.

Copy link
Member

Choose a reason for hiding this comment

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

RandomMessageNamespace 👍 makes sense to me

rach-id
rach-id previously approved these changes Oct 5, 2022
View reviewed changes
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

🚀

@raphaelts3 raphaelts3 force-pushed the remove-duplicated-randomValidNamespace branch from 073dc7b to 0db4998 Compare October 5, 2022 18:54
rootulp
rootulp requested changes Oct 5, 2022
View reviewed changes
pkg/nsgenerator/random_ns_generator.go Outdated Show resolved Hide resolved
@raphaelts3 raphaelts3 force-pushed the remove-duplicated-randomValidNamespace branch from 9620f00 to 1b93774 Compare October 6, 2022 13:53
rootulp
rootulp approved these changes Oct 6, 2022
View reviewed changes
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution @raphaelts3 and persisting through rounds of feedback!

@evan-forbes
Copy link
Member

evan-forbes commented Oct 6, 2022 •
edited
Loading

[optional][should be done in a different PR]
I don't want to block this, since I haven't participated in reviews so far. This is just a nit comment that we should address in a different PR

It makes sense to me tho that the new namespace package is moved to the testutil package, given that this is exclusively a testutil function

@rootulp
Copy link
Collaborator

rootulp commented Oct 6, 2022

It makes sense to me tho that the new namespace package is moved to the testutil package, given that this is exclusively a testutil function

I took a stab at this yesterday but it caused cyclic imports. Agreed that there may be a more desirable long-term location for this util

@evan-forbes
Copy link
Member

but it caused cyclic imports.

the new namespace package is moved to the testutil package

while this doesn't resolve the annoying name conflict, it should fix the cyclic imports, no?

@rootulp rootulp merged commit a3830e3 into celestiaorg:main Oct 7, 2022
@rootulp
Copy link
Collaborator

rootulp commented Oct 7, 2022

oh sorry, I meant that I tried moving the func RandomValidNamespace to the testutil package but I think your proposal is to move the namespace package inside the testutil directory.

@rootulp rootulp mentioned this pull request Oct 7, 2022
Merged
rootulp added a commit that referenced this pull request Oct 8, 2022
@rootulp
chore: move namespace pkg under testutil directory (#846)
be5406b 
Maybe resolves
#838 (comment)
if I interpreted it correctly
rach-id pushed a commit to rach-id/celestia-app that referenced this pull request Nov 16, 2022
@raphaelts3 @rach-id
Remove duplicated random valid namespace (celestiaorg#838)
de6cc46 
Closes celestiaorg#836

Hey, @rootulp I'm not an expert go, so I don't know if the move I've
made is the best possible, but it did its job. That said if you have any
suggestions to improve that I'm open to adjusting it.

PS: Since the repo doesn't contain the tag `hacktoberfest`, would be
possible to label this PR with `hacktoberfest-accepted` so it can be
validated there by the rule below?
> PR/MRs that also have the “hacktoberfest-accepted” label cannot be
marked as spammy via a label.
rach-id pushed a commit to rach-id/celestia-app that referenced this pull request Nov 16, 2022
@rootulp @rach-id
chore: move namespace pkg under testutil directory (celestiaorg#846)
d31ecf8 
Maybe resolves
celestiaorg#838 (comment)
if I interpreted it correctly
cmwaters pushed a commit to celestiaorg/go-square that referenced this pull request Dec 14, 2023
@rootulp
chore: move namespace pkg under testutil directory (#846)
bba7c38 
Maybe resolves
celestiaorg/celestia-app#838 (comment)
if I interpreted it correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rootulp rootulp rootulp approved these changes

@rach-id rach-id rach-id left review comments

@evan-forbes evan-forbes Awaiting requested review from evan-forbes

Assignees

@raphaelts3 raphaelts3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove duplicate definitions of randomValidNamespace
4 participants
@raphaelts3 @evan-forbes @rootulp @rach-id