-
Notifications
You must be signed in to change notification settings - Fork 954
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
fix: add validation on email and discord handle in cli #2805
Conversation
4cbe5c5
to
d2948e6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2805 +/- ##
==========================================
- Coverage 53.95% 53.91% -0.04%
==========================================
Files 308 308
Lines 100018 100084 +66
==========================================
- Hits 53967 53965 -2
- Misses 46051 46119 +68 ☔ View full report in Codecov by Sentry. |
@phy-chain thanks for starting this. It is more correct and appropriate to enforce these constraints in the user VP ( We do a similar thing already in the native Governance VP with the restriction of the size of a governance proposal content. Check out Make sure to include a test as well. |
Ah yes, I did see that, That was my source of inspiration for the MetadataValidation structure actually. But I didnt get how that worked with VP thing. |
@phy-chain I'm happy to help at some point soon since this is important, but I don't have the bandwidth for probably a couple days. |
@brentstone No rush, Im glad to help, even though Im a total newby at Rust (Im a frontend dev first and foremost). I've started a test case but on that, it's a complete mystery how to generate "test data" that's relevant, and test against it. So, i've copied and existing test and added some TODOs as a reflexion basis. Feel free to ping me when you have time (better to do so on Discord cause I might not see notifications here). It can also be after the SE if you're too busy. |
hey @phy-chain, in some planning today we decided that the priority for this is higher than previously thought, so I went ahead and made a quick PR #2845 to complete this. I'll close this PR for now, but perhaps keep your branch around in case. Thanks for the effort and for identifying #2800! |
Lol ok, that's a way to go at it 😅 500 chars limit for all. Still better than nothing I guess :D |
Describe your changes
Indicate on which release or other PRs this topic is based on
This has been reported in : #2800
Checklist before merging to
draft