Skip to content
This repository was archived by the owner on Jul 4, 2025. It is now read-only.

feat: add new options to string scheme#115

Closed
bitkidd wants to merge 1 commit intoadonisjs:developfrom
bitkidd:develop
Closed

feat: add new options to string scheme#115
bitkidd wants to merge 1 commit intoadonisjs:developfrom
bitkidd:develop

Conversation

@bitkidd
Copy link
Copy Markdown

@bitkidd bitkidd commented Jun 9, 2021

Proposed changes

  • Add case (lowerCase, upperCase, camelCase, snakeCase, dashCase, pascalCase, capitalCase, sentenceCase, dotCase, titleCase, noCase) options.
  • Add pluralize, singularize and condenseWhitespace options

Types of changes

What types of changes does your code introduce?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

… to string

- Add case (lowerCase, upperCase, camelCase, snakeCase, dashCase, pascalCase, capitalCase, sentenceCase, dotCase, dotCase, titleCase, noCase) options.
- Add pluralize, singularize and condenseWhitespace options
@thetutlage
Copy link
Copy Markdown
Member

Looks great. However, I suggest converting it to a rule. It can be one rule that is changeCase or can be multiple rules. I think one rule is fine.

Why convert it to a rule?

I am also going to convert the trim and escape options to rules, so that the end user can decide when to apply them. For example:

Right now, the minLength and the maxLength rules validate the original value and not the trimmed value (if the option is used). For some, this behavior is fine and for others this behavior is problematic.

To solve this, if I convert the trim option to a rule, then the end user can decide when to apply based upon their scenario.

schema.string({}, [
  rules.trim()
  // Apply trim before
  rules.minLength(4)
])

schema.string({}, [
  rules.minLength(4)
  // Apply trim after
  rules.trim()
])

I know that your rule is not prone to this behavior. But I will love to unify the API and get rid of the options all together

@bitkidd
Copy link
Copy Markdown
Author

bitkidd commented Jun 15, 2021

It is a great idea to convert options to a separate set of rules, but should it be really a rule?

Feels like a rule is a logic operator, it could be a complex one, but this operator semantically should answer a single question, for example for rule.email it asks a question if the string is email and then answers true or false.

Wouldn't it be better to implement something like modifiers that can be used in the same array with rules and basically will just mutate the values, but semantically you will know what to wait from them, they get a value and modify, the naming is just an example, it can be anything really, sanitizers, modifiers, mutators.

The actual use may look like this:

schema.string([
  modifiers.trim()
  // Apply trim before
  rules.minLength(4)
])

schema.string([
  rules.minLength(4)
  // Apply trim after
  modifiers.trim()
])

What do you think @thetutlage?

@thetutlage
Copy link
Copy Markdown
Member

Yup, I like the modifiers approach. Now this brings me to an internal discussion I had with @targos and @RomainLanz.

I think the validator API needs some love. The most radical approach will be to use a chainable API like the following.

schema
  .string()
  .email()
  .trim()
  .minLength(10)
  .changeCase('camelCase')

I call it radical, coz it is a completely different API and I am not interested in introducing a breaking change at all.

But, then I have other ideas in mind like the ability to define a custom field name which is different from the schema property name. For example:

firstName: schema.string().field('first_name')

With the chainable API it is simple, coz each function is not part of any group. Whereas with existing API, what will we call this method? Like we call rules for validation rules, modifiers for mutating the field value. What is this 3rd thing called?

firstName: schema.string({}, [
  what.field('first_name')
])

Any suggestions or ideas?

@bitkidd
Copy link
Copy Markdown
Author

bitkidd commented Jun 15, 2021

Current validator implementation is one of my beloved functionalities in adonis really, as it allows in a very structured and predictable manner build data schemes.

If speaking about changing field name, we can easily add this to modifiers group, as it semantically modifies/mutates data, the approach is not ideal but it will not lead to a breaking change.
The new functionality can be added leaving old options api that will allow developers to adapt and prepare.

As for chainable API, this looks good too, but I feel it a bit less structured in comparison to what we have now.

@bitkidd
Copy link
Copy Markdown
Author

bitkidd commented Jun 15, 2021

The problem with modifiers approach rises with scheme.file where there are real options, that cannot be categorised as a rule or modifier, for example:

schema.file({
  size: '2mb',
  extnames: ['jpg', 'gif', 'png'],
})

In this case chainable API resolves the problem.

scheme.file().size('2mb').extnames(['jpg', 'gif', 'png'])

@thetutlage
Copy link
Copy Markdown
Member

Great. @RomainLanz also thinks the current validator API is more logical than the chainable API. So let's not change too many things and add support for modifiers.

Now, if you need this feature immediately, then we can go ahead and implement it. Otherwise, I will suggest going the RFC route to solidify the API even further.

If you want, I can initiate the RFC this weekend and then we can exchange ideas and finalize it

@bitkidd
Copy link
Copy Markdown
Author

bitkidd commented Jun 15, 2021

I don't need the change right away.
Let's initiate RFC and discuss the change there, so maybe other ideas arise.

@bitkidd
Copy link
Copy Markdown
Author

bitkidd commented Jun 15, 2021

If you want, I can do the RFC tomorrow and submit it, the repo is: https://github.com/adonisjs/rfcs
Right?

@thetutlage
Copy link
Copy Markdown
Member

Sure, that will be great. Just checkout the websocket rfc as an inspiration

@stale
Copy link
Copy Markdown

stale Bot commented Aug 14, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the Status: Abandoned Dropped and not into consideration label Aug 14, 2021
@RomainLanz
Copy link
Copy Markdown
Member

Here is the link to the RFC: adonisjs/rfcs#40.

Closing in the meanwhile.

@RomainLanz RomainLanz closed this Aug 15, 2021
@thetutlage thetutlage removed the Status: Abandoned Dropped and not into consideration label Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants