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

add sometimes rule & undefined concept #79

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

cristiancalara
Copy link
Contributor

This adds the "undefined" concept from an array to the data object. It also adds the 'sometimes' rule to accompany the main use case i.e. partial updates in a form request.

class ProductUpdateData extends DataTransferObject
{
    public function __construct(
        public string|Undefined $name,
        public bool|Undefined $available,
    ) {
    }
}

This will allow a partial data request, where either name or available can be omitted from the payload. When the data object is transformed back into an array, the keys that were not sent, will not be present.

@cristiancalara
Copy link
Contributor Author

A few notes / questions:

  1. I'm considering making the Undefined class a singleton. That would allow for equality checking and a performance boost?
  2. RemoveUndefinedTypeProcessor was a duplicate from RemoveLazyTypeProcessor with the only thing different being the class that it removes. I'm thinking of merging this into one class that will tackle both use case? (maybe just send the class type as a param)
  3. Used "isUndefineable". Started with "isUndefined" for the type, but thought that this concept is more similar to the "nullable" concept so even though it doesn't sound that great, went with "isUndefineable". Open to suggestions on this.
  4. Having a default value (using promoted properties) alongside Undefined does not make sense. Not sure if we should maybe implement some sort of warnings? I mean, it doesn't have any impact on the functionality, but I guess it would be a nice thing to add?

@rubenvanassche
Copy link
Member

Hi @cristiancalara,

I'm loving this PR!

As for your comments:

  1. I don't think that's necessary, we can change this later if it would become a real bottleneck
  2. Merging these both TypeProcessors sounds like a good idea let's call it RemoveInternalDataInternalsTypeProcessor? That would be a breaking change, but we're planning on releasing data V2 in a couple of weeks so that won't be an issue.
  3. Like the idea, let's keep it like that
  4. In my opinion developers should be smart enough not to do this 😄, I think adding extra checks will only make the code more complicated so let's leave it as it is.

So, I'm gonna merge this branch in V2 and play a bit more with it to check if I can find any edge cases.

Thanks!

@rubenvanassche rubenvanassche changed the base branch from main to v2 January 26, 2022 14:02
@rubenvanassche rubenvanassche merged commit e4d25df into spatie:v2 Jan 26, 2022
@SagarNaliyapara
Copy link

when v2 will be released? @rubenvanassche I'll need this in my partial update endpoints

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.

3 participants