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

Make code output stable across multiple runs #41

Open
dktapps opened this issue Oct 6, 2021 · 7 comments
Open

Make code output stable across multiple runs #41

dktapps opened this issue Oct 6, 2021 · 7 comments

Comments

@dktapps
Copy link
Contributor

dktapps commented Oct 6, 2021

Is your feature request related to a problem? Please describe.
Currently, this library relies on uniqid() quite a lot to ensure uniqueness in generated code. This is quite annoying when the generated code is in a git repository, because schema code changes every time my build script runs even if the schema itself was untouched.

Describe the solution you'd like
For class names, I've had some success with a custom class name generator that suffixes with md5(json_encode(propertySchema)) instead of uniqid(). However, there's some additional places that I can't touch without modifying the library itself:

  • ArrayItemValidator generates variable names using uniqid(). I'm not actually very clear why this is needed at all, since validators are anyway split up into separate methods per property.
  • AbstractPropertyProcessor uses uniqid() to generate class names for rendering dependencies. Again, I'm not very clear why this is needed - isn't ClassNameGenerator enough to take care of this?

Additional context
image

@dktapps
Copy link
Contributor Author

dktapps commented Oct 6, 2021

For the ArrayItemValidator case it seems like md5(propertyName) is sufficient. I checked nested arrays and their property names are item of {actualPropertyName}, so this seems fine.

@wol-soft
Copy link
Owner

wol-soft commented Oct 6, 2021

I've had a quick view: for the AbstractPropertyProvider I think the uniqid is not necessary, as you already stated in the issue description.

The ArrayItemValidator on the other hand currently needs the suffix. You're right, each property is evaluated in a single function so conflicts won't occur. But if a property contains a nested array the validation for the property will contain nested array validators in a single function which will collide and overwrite each others invalid items collection (if the variable names are identical, eg. both use $invalidItems).

Consequently some kind of 'nested level counter' or something similar would be required to generate a reproducable suffix for the variable names. I'll have a look into it the next days.

For all my usages of the library, I don't commit the generated code (as it's just a duplication in my opinion. All information is already committed in the JSON-files used to generate the classes). I run my generate-script as a build step of the applications (like composer install to not commit the vendor directory). Is there a specific reason why you commit the generated code?

@dktapps
Copy link
Contributor Author

dktapps commented Oct 6, 2021

Is there a specific reason why you commit the generated code?

I'm currently experimenting with this library and it seemed like the simplest way to do it right now, because otherwise deploy involves two extra steps (install dev deps, generate code, then install non-dev deps). I haven't settled on how I'm going to use it right now.

The ArrayItemValidator on the other hand currently needs the suffix. You're right, each property is evaluated in a single function so conflicts won't occur. But if a property contains a nested array the validation for the property will contain nested array validators in a single function which will collide and overwrite each others invalid items collection (if the variable names are identical, eg. both use $invalidItems).

I did some experimentation and found that a hash of this seems to be sufficient for nested arrays.

@wol-soft
Copy link
Owner

wol-soft commented Oct 6, 2021

I did some experimentation and found that a hash of this seems to be sufficient for nested arrays.

Yes, you are absolutely right. Clean and simple solution. I've added it in 1b7a21a to the master as well as a patch for the AbstractPropertyProvider. You may test it with the latest master version.

Thanks for the hint 😃

@dktapps
Copy link
Contributor Author

dktapps commented Oct 6, 2021

Thanks. Only thing that remains (AFAIK) is the ClassNameGenerator, but I solved that with my code using a custom impl that just md5's the contents of the json schema. There's probably a better way to do it though (is there a way to get the full property path in there?).

@dktapps
Copy link
Contributor Author

dktapps commented Oct 7, 2021

Is there a specific reason why you commit the generated code?

I just want to mention something in addition to what I said before, which is that as well as making initial install as root project more complicated, codegen on install is not an option if the package may be used as a library, since dev deps won't be available. In that case, the only choices are to either commit the code or to depend on the codegen tool as a non-dev dep, neither of which are ideal. I think committing the code is the lesser evil in this case.

@wol-soft
Copy link
Owner

wol-soft commented Oct 8, 2021

Yes, using this library in a library context currently has no proper solution. Maybe it's possible to wrap the generate script into a Composer plugin. I've not yet tested this idea.

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

2 participants