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

Help with maintenance #201

Open
Keats opened this issue Mar 15, 2022 · 33 comments
Open

Help with maintenance #201

Keats opened this issue Mar 15, 2022 · 33 comments

Comments

@Keats
Copy link
Owner

Keats commented Mar 15, 2022

Hey everyone,

As you might have noticed I'm not very reactive with issues in that repository. The main reason is that I'm not (actually never have) used this crate myself so it's not really a priority for me.
I would be happy to promote some new maintainers to handle the new features/fixes they need rather than being the bottleneck. I will still be available if needed, especially for releases etc of course.

@Keats Keats pinned this issue Mar 15, 2022
@krojew
Copy link
Contributor

krojew commented Mar 24, 2022

I might be able to help you. I already maintain 4 crates.

@Keats
Copy link
Owner Author

Keats commented Mar 28, 2022

That would be great! The main thing to do really is to look at the PRs to see what's missing from them/whether we should close them or not.

@krojew
Copy link
Contributor

krojew commented Mar 30, 2022

As far as I can see, most PRs require test fixes and/or rebasing. The only ones, which can be merged now are #140 and #203. I'm not sure #189 is a good idea, since it introduces potential to accidentally ignore fields for validation.

@Keats
Copy link
Owner Author

Keats commented Mar 31, 2022

I'm not sure #140 makes sense to have in the library itself

@krojew
Copy link
Contributor

krojew commented Mar 31, 2022

Domain validation sounds useful, so it might be a good idea to have it alongside the email one.

@IniterWorker
Copy link
Contributor

@krojew and @Keats what do you propose?

  • I don't want to switch every time a new outsider comes. So, pick one direction, please.
  • @Keats was not happy with a "skip" validator.

@Keats
Copy link
Owner Author

Keats commented Mar 31, 2022

I think your PR direction is fine, it only allows for more validation since you would get a compile-time error otherwise. I'm not sure what @krojew means by accidentally ignoring fields for validation.

@krojew
Copy link
Contributor

krojew commented Mar 31, 2022

I might have misunderstood how that PR works.

@krojew
Copy link
Contributor

krojew commented Apr 4, 2022

What's the plan on moving forward?

@krojew
Copy link
Contributor

krojew commented Apr 5, 2022

To amend my question - if you don't want to support the crate, I can take over, since I'm very much interested in keeping it alive.

@Keats
Copy link
Owner Author

Keats commented Apr 5, 2022

I mostly need help for code reviews and guidance for new features (eg #192), although potentially slow. I'm still there for some reviews/merging/releasing.

I think the current master branch will get a new release this week unless someone takes over #192

@IniterWorker
Copy link
Contributor

IniterWorker commented Apr 5, 2022

@Keats could you reviews #189.

I will try to get into #192 end of the week. I need more pratice on writting macro. But, yes, it is not for this release.

@Keats
Copy link
Owner Author

Keats commented Apr 6, 2022

#189 is already merged!

@IniterWorker
Copy link
Contributor

I am seeing staled issue. Should we instore a closing issue policy ?

@Keats
Copy link
Owner Author

Keats commented Apr 10, 2022

I'm not a big fan of stale issues - some old issues are still valid. We should rather go through the issues one by one and close if needed.

@bradleyess
Copy link

Also happy to help out!

@Keats
Copy link
Owner Author

Keats commented Apr 27, 2022

Thanks! Any thoughts on #206 and #205?

@krojew
Copy link
Contributor

krojew commented May 5, 2022

#205 looks quite good - it will greatly improve extensibility and ease maintenance.

@Keats
Copy link
Owner Author

Keats commented May 5, 2022

It would need someone willing to implement it though, it would be a pretty big PR.

@IniterWorker
Copy link
Contributor

#170 - We don't have the macro logic to support standard Ref and Cell. Any thoughts about this feature?

@krojew
Copy link
Contributor

krojew commented May 15, 2022

I think it's an another argument for implementing #205

@IniterWorker
Copy link
Contributor

Agree. We should be able to be backward compatible for at least all validator's functions. But, I have some concerns about extract_custom_validation and custom functions.

Should we focus on #205?

@krojew
Copy link
Contributor

krojew commented May 16, 2022

In my opinion, #205 looks promising and should be prioritized.

@Keats
Copy link
Owner Author

Keats commented Jun 27, 2022

I think with #205 , it might be worth doing a full rewrite of this crate. Probably easier than trying to fit it in and I'm sure the crappy code that was me learning proc macro can be written much better.

@IniterWorker
Copy link
Contributor

I started to think about #205 as a refactoring without breaking end-user macro-API.

  • Should we have a conservative approach?

However, I don't have too much time these days.

@Keats
Copy link
Owner Author

Keats commented Jun 27, 2022

I'm not expecting the end-user api to change but all the internals. The issue with a conservative approach is that #192 needs deep refactoring either way, so it's easier/faster to start from scratch imo than bolting on more features

@DXist
Copy link
Contributor

DXist commented Jul 25, 2022

I noticed this issue and decided to mention my PR here to get potential feedback from active crate users .

I'm interested whether the feature is useful enough to integrate into the validator crate.

@DXist
Copy link
Contributor

DXist commented Mar 20, 2023

What do you think about length_utf16 validator?

I think it may be useful to match length constraints in backend and frontend validators - JavaScript returns length in UTF16 characters while length validator checks Unicode codepoints.

@jprochazk
Copy link

jprochazk commented Mar 26, 2023

Seeing as the general consensus is that a rewrite makes sense, and that large breaking changes are unavoidable, I figured I'd try to start from scratch with a new API. I gave it a shot over the weekend, and the result of this is garde. I gathered various feature requests that I found here and either implemented them or created an issue for them.

It's mostly the same in terms of usage. The core is a Validate trait and its validate method. The implementation of this trait may be automatically derived.

Documentation is a little bare at the moment, so tests are the best place to look for usage examples.

The major differences are:

  • Validation rules are traits, so they may be implemented for custom types. (Use traits for validation rules #205)
  • The generated trait accepts a Context type, which is passed to each custom validator. This allows arbitrary customization to the validation process, and it opens the space up for use-cases like localization of error messages.
  • The derive macro supports enums. (Support enum #77)
  • Added Valid<T> and Unvalidated<T>, which implement the typestate pattern. (Enforce validation with typestate #185)
    Unvalidated may be created from arbitrary values, but the only way to create Valid is to call validate on Unvalidated. Unvalidated also implements serde::Deserialize, so it may be used in integrations with web frameworks.
  • Rules are responsible for generating error messages, and the resulting errors do not receive params afterwards. This means that sensitive should no longer be necessary, but I might also be totally wrong about this approach, not sure about this!

Lots of remaining work to do, but as far as I can tell, the only features which are missing for feature parity with validator are the nested and required rules. I'm really interested in seeing what people think!

@Keats
Copy link
Owner Author

Keats commented Mar 26, 2023

There are some neat ideas that I should definitely take for validator.

Rules are responsible for generating error messages, and the resulting errors do not receive params afterwards. This means that sensitive should no longer be necessary, but I might also be totally wrong about this approach, not sure about this!

Sometimes error messages are not generated on the server and you might accidentally log things if there is a default error message (I'm not sure I haven't looked into that in garde).

The name garde is a tiny bit weird because i associate it with to keep first before to guard which is both garder in French but that's not a big deal.

@jprochazk
Copy link

jprochazk commented Mar 26, 2023

Sometimes error messages are not generated on the server and you might accidentally log things if there is a default error message (I'm not sure I haven't looked into that in garde).

garde does not log the values at all. The error messages are generated only from the constraint parameters, such as:

#[derive(Validate)]
struct Test {
  #[garde(range(min=100))]
  field: u64,
}

println!("{:?}", Test { field: 0 }.validate(&()).unwrap_err().flatten());
// prints:
// [("value.field","lower than 100")]

The name garde is a tiny bit weird because i associate it with to keep first before to guard which is both garder in French but that's not a big deal.

It seemed appropriate to me, because I typically use validation in a "guard branch", like:

if let Err(e) = thing.validate() {
  return Err(e);
}
// or just
thing.validate()?;

happy_path();

But I don't know any French, so fair enough!

@Keats
Copy link
Owner Author

Keats commented Mar 27, 2023

garde does not log the values at all. The error messages are generated only from the constraint parameters

Yeah that's definitely one way to do it and is probably the best way to handle it tbh.

@IniterWorker
Copy link
Contributor

But I don't know any French, so fair enough!

The name garde is a tiny bit weird because i associate it with to keep first before to guard which is both garder in French but that's not a big deal.

I am a native french speaker, garde is good enough. If you say la garde in french then it means your guard or a guard watching. And, I think it would fit the application goal protected by watching.

Example from https://www.wordreference.com/fren/garde

  • À la boxe, il ne faut jamais baisser sa garde.
  • Never drop your guard when boxing.

@jprochazk, I think we should have some community chat to work together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants