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

message/validation: rename package #1864

Open
nkryuchkov opened this issue Nov 21, 2024 · 5 comments
Open

message/validation: rename package #1864

nkryuchkov opened this issue Nov 21, 2024 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@nkryuchkov
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Currently, message validation is located under .../message/validation the package is called validation. When we import it, it's referenced as validation and it might not be clear what validation is meant

Describe the solution you'd like
Perhaps something like msgvalidation would be clearer and wouldn't increase the name size by a lot

Describe alternatives you've considered
Call it messagevalidation but it seems quite long

Additional context
We probably don't need to touch genesis message validation as we will remove it, this will also decrease the amount of conflicts with the PR that removes pre-fork code

@nkryuchkov nkryuchkov added enhancement New feature or request good first issue Good for newcomers labels Nov 21, 2024
@nkryuchkov nkryuchkov changed the title Rename message validation package message/validation: rename package Nov 21, 2024
@BavyaMittal
Copy link

Hello my name is Bavya and I am a software engineering student looking for a good first issue. I would like to try resolving this issue. Can I work on this if no one else is assigned yet?

@nkryuchkov
Copy link
Contributor Author

@y0sher @moshe-blox @iurii-ssv @olegshmuelov can you please share your opinion on this, whether you agree?

@nkryuchkov
Copy link
Contributor Author

Hello my name is Bavya and I am a software engineering student looking for a good first issue. I would like to try resolving this issue. Can I work on this if no one else is assigned yet?

@BavyaMittal Hi Bavya, thanks for your suggestion, and sorry for the delay. I need to sync with the teammates to decide whether we are going to do this

@iurii-ssv
Copy link
Contributor

iurii-ssv commented Dec 5, 2024

Taking a step back to think about Golang package-naming in general - it's designed such that you

  • always need to consider package+struct/func combination rather than package name in isolation,
  • and more often than not, splitting a package into several smaller ones doesn't give best results (and we end up with validation.Something where it's not clear what validation we are referencing)

so, instead of just renaming validation package I'd suggest we look into maybe

  • flattening package structure itself (instead of message/validation we could move everything into message package, so that validation.MessageValidator becomes message.Validator and so on)
  • and maybe also renaming message package into p2pmsg or similar (if that makes sense) to further clarify this, and maybe even moving/grouping this p2pmsg package with other p2p functionality (since it seems all it's doing is working with p2p messages, so why have it at the root of ssv repo then if it's just an implementation detail).

But if we don't want to restructure the packages involved, using msgvalidation would probably be better than just validation (as it currently is)

@nkryuchkov
Copy link
Contributor Author

Perhaps, we should agree on a project-wide convention before implementing this because message validation is not the only place where we need to think about this.

@iurii-ssv I agree with what you said about the message validation, indeed we can move everything to the message package. Regarding other packages where flattening is not to easy or impossible, I think we could use the approach with prefixes like having message/msgvalidation path and msgvalidation package name (I think we mostly follow this approach). This is something similar to what the Go standard library uses, e.g.:

  • golang.org/x/net/netutil (not net/util)
  • hash/maphash (not hash/map)
  • io/ioutil (not io/util)
  • http/httputil (not http/util)
  • testing/slogtest (not testing/slog)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants