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

Allow fixtures to have closures as properties #13

Closed
wants to merge 15 commits into from

Conversation

whatyoubendoing
Copy link

  • Allow properties that are closures to be evaluated at generation
    time.
  • Add tests for this

Benjamin Haines added 6 commits April 20, 2015 12:03
Rename BaseDriver to the more implicit PDODriver. Remove Str class from
being able to be passed in. Str generally won’t change.
- Add the ability to generate keys from a “key generator”
- Include the default SHA1 and a Rails-like key generators
- Add docs & tests
- Allow properties that are closures to be evaluated at generation
time.
- Add tests for this
B3njamin and others added 9 commits April 20, 2015 23:20
Rename BaseDriver to the more implicit PDODriver. Remove Str class from
being able to be passed in. Str generally won’t change.
- Add the ability to generate keys from a “key generator”
- Include the default SHA1 and a Rails-like key generators
- Add docs & tests
@clphillips
Copy link
Contributor

+1

But I don't think project owner is accepting PRs on master (per the Readme).

@tabennett
Copy link
Contributor

I don't have a problem with accepting these into master. I think this is a pretty good idea and brings a bit more flexibility to the package. I'm still looking through these other PR's though. Please be patient.

@whatyoubendoing
Copy link
Author

Im currently working on a version in branch https://github.com/onceit/fixture/tree/next. It seems as if functionality is broken / not documented correctly, also would like to see some additional functionality. Im moving pretty fast and breaking stuff so unlikely that ill be able to submit pull requests for each piece of changed functionality. Happy to merge back in as a version 1.x, if not I can fork the library Trying to stick with similar functionality to rails fixtures.

@clphillips
Copy link
Contributor

I'd still like to see this PR merged. It's a big step in the right direction.

@tabennett
Copy link
Contributor

I really like the idea of having closures, PSR4, Key Generators, etc. However, I can't merge this because of the following reasons:

  • Things have been renamed that I don't want renamed (BaseClass -> PDO) without any dialogue or discussion.
  • The base class for all drivers should be an abstract class. It's not designed to be instantiated as a standalone class (regardless of the fact that it has no abstract methods).
  • The Str class should be not be instantiated inside of the constructor of the base driver class. I don't want the base class to have a constructor at all and if does then the an instance of Str should be injected in.
  • This package should not require Illuminate/Support/Str 5 exclusively. In fact, it probably shouldn't use it at all. I need this library to work independently of Laravel. That class was only being used for simple things like studly case to snake case, etc and it was used at a time whe
  • Your syntax for writing tests is not the same as the syntax currently used in this package.

Those a just a few issues I have with this. The biggest issue (and most frustrating) is that there are several features that I do want to add from these PR's (closures, PSR-4, Key generators, etc) but I can't because these all seem to based off of the same root branch and are all clumped together. There are also many arbitrary changes that appear to have been hastily made (name changes, dependency change, etc) without any discussion or forethought. So I'm going to merge in the first PR from this group (the dependencies update, etc). These other I'm not going to merge in and will probably have to implement myself in order to get the same features (and that sucks).

@clphillips
Copy link
Contributor

@tabennett I can work on pushing independent PRs for Key generators, PSR-4, and closures. Would you want those on development branch or master?

I agree about the illuminate support. At most, those should be "suggested" packages via composer. One reason I like this lib is because it's lightweight.

@whatyoubendoing
Copy link
Author

I guess Ive jumped the gun a little, originally I had these broken in to different pull requests. I do understand why it must be frustrating and why some of the changes do need to be discussed.

I can revisit maybe holding fire & creating separate pull requests for each piece of work. Just want to use this on a project and in its in its current state the library isn't functioning as per the readme or as excepted. I also may just continue with my fork. My comments & concerns below.

  • The rename of BaseClass to PdoClass was done for semantics. The BaseClass relies on a PDO object. Say we had a driver for Redis or Mongo this would not require all the methods that are currently implemented on the BaseObject.
  • Schemas that use foreign keys constraints fail as fixtures are loaded in regardless of order
  • BelongsToMany relationships don't work as expected pivots with timestamps aren't populated etc.
  • Most of BelongsToMany syntax its done through string manipulation rather then just using the PHP array syntax to solve these problems
  • The package requires the Faker library
  • There is no point of reimplementing endsWith. Seems fine that the str class is required by both the Standard and Eloquent. After all the library isn't the entirety of laravel
  • Access the str class is don't in 2 different ways through the functions e.g. camel_case and through the class
  • No tests for postgres or mysql
  • Syntax for tests are using snake_case rather then PSR4 camelCase
  • The Fixture class requires it to initialised as a singleton rather then letting the developer decide how they want to use it.

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