bchavez
Add Vehicle, Music, and Person to Parse method
Issue/Feature
Faker contains a parse method however it is missing implementations for Vehicle, Music, and Person.
Implimentation
Vehicle:
- Added to list of Mustache Methods passed into Parse method
- Added unit tests to check that values can be returned from parse
Music
- Added to list of Mustache Methods passed into Parse method
- Added unit tests to check that values can be returned from parse
Person Added wrapper class that initializes a lazy instance of Faker. This is because parsing Person directly into the list of Mustache methods directly would add an additional call to Random which messes up the seed for other unit tests. doing it this way also allows for methods inside the person wrapper to be named the same as the properties in the actual person class. Made changes to the Tokenizer so that it wouldn't pick up internal classes. This is so the PersonWrapper class wouldn't be seen or used outside of it's intended use for parse. (I imagine it might get confusing for users if there are two Person methods available). Added unit tests to test all values returned (except for DOB which I'm not sure how best to test). And tests to make sure that the parse method still functions correctly with the Person class given its unique functionality in contrast to the other classes.
In my eagerness to add the functionality to complete the Parse method, I left in some unnecessary code. Hopefully my implementation is a bit cleaner now.
I noticed that some more recent pull requests have been looked at. Just want to check if there is anything I have missed or need to do for mine. Also happy to try and rework the code if there is an issue with it. Its a relatively small change but I think it could be valuable for people like myself who utilize the parse method.
Sorry to be a pain, I know you're probably very busy.
Love the library. It's such an amazing tool.
Hi @Perks-of-Being-a-Cauliflower, thank you for the PR.
Out of the gate, I think I would take the changes for Music and Vehicle; but the PersonWrapper, I need to think about and noodle on that implementation a bit.
If you want to break out the PR for Music and Vehicle into a separate PR, I could approve that faster. However, I'm not sure about the PersonWrapper.
The main reason I'm a little hesitant is that this particular implementation is adding more "context state" that needs to be tracked and reset on each new context. While it is OK-ish and looks like it works; I would prefer to keep the management of "context tracking state" to a minimum. So, I'm wondering if there's a better way where we might not need a PersonWrapper and additional context state.
Perhaps,
https://github.com/bchavez/Bogus/pull/602/files#diff-44577bc608b694912be18cf40e8addfcee7373f5b33d899603bfbe1ee2b66a6bL21
there's could be additional dedicated registration for Person fields that create and augment the existing registrations with new MustashMethod that are Func<T>s. My hesitation comes from seeing the existing code changes seems like we are shoehorning [RegisterMustasheMethods] to make it work with a special case Person class fields.
Since Person is kind-of a special case object in Bogus, I think it's also okay for it to be a special case registration that it has its own kind of registration without requiring [RegisterMustasheMethods].
Feel free to keep this PR open, and if you decide to work on a different approach, please open a new PR so we can compare both approaches.
Thanks for the reply @bchavez.
Sorry I haven't done anything on this in months, life things happened. I have been pondering your comment and trying to conceptualize what you want but I think my unfamiliarity with the registration code gives me pause since the deeper I go I feel like the more chances I have to meddle with code that could affect other functionality.
The reason I went for a wrapper class was because it felt wasteful to redefine the entire Person class and I didn't want to risk negatively affecting the existing Person class. Also, I was hoping to reduce overhead should Person ever change. It also allowed me to redefine the properties as methods without naming conflicts. That all being said I get your point about not wanting to handle two contexts. I had issues reusing the old context since it would cause Person to be generated twice on initialization which caused all the expected unit test values to change and therefore fail most of the unit tests.
That being said I will try to look further into the registration to see if I can figure out a more elegant way to achieve what I did with the wrapper.
I have split the Music and Vehicle changes to a separate pull request as you requested and will start on a new PR/Branch to try out the other implementation.