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

conditionally required field, requiredWhen validator spec #223

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

NewBieCoderXD
Copy link
Contributor

heavily inspired by @neezer, #78, and #81

How it works:
instead of validate requirement based on raw env, we clean the env, then check if it is required.
requiredWhen is a boolean function that takes cleaned env then returns if the field is required.

I think this would get rid of most of the problems by using cleaned env instead of raw.

let me know what you think :)
(this is actually my first few PR outside of university, so I'm sorry if I've done anything wrong)

@natterstefan
Copy link

natterstefan commented Jul 24, 2024

What a timing. I need this :D Thank you @NewBieCoderXD.

@af
Copy link
Owner

af commented Jul 25, 2024

Thanks for the PR, this looks promising! I'll need to refresh myself with the history on this feature request and do a review, will try and get to that in the next couple days

const server = (env.USE_TLS)? http.createServer(serverBody): https.createServer({
key: fs.readFileSync(env.TLS_KEY_PATH),
cert: fs.readFileSync(env.TLS_CERT_PATH)
},serverBody);
Copy link
Owner

Choose a reason for hiding this comment

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

Could you roll back the changes to this example file? IMO they make this very simple file harder to follow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing!

@af
Copy link
Owner

af commented Sep 23, 2024

Sorry for the slow response, this one got away from me! I have a couple things to address but will push them in a separate commit. Thanks for the contribution!

@af af merged commit 1d9d237 into af:main Sep 23, 2024
@maxcbc
Copy link

maxcbc commented Nov 14, 2024

Hi @af,

Now that this PR has been merged. Will it go out in an 8.1 release?

I'm really keen to use this new feature. Thanks to both yourself and of course @NewBieCoderXD for getting this in.

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

Successfully merging this pull request may close these issues.

4 participants