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

UseConstructor(), SkipConstructor() and fixing StrictMode() error #338

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Basyras
Copy link
Contributor

@Basyras Basyras commented Nov 23, 2020

@bchavez Promised PR from #334
➕ Adding features for specifying constructors: UseConstructor() and SkipConstructor()

  • UseConstructor() also provide overload for specifying if paremeterless constructor is private/protected
    U have already discussed if this should be in Bogus here: Using Faker[T] with private constructor and private property setter #213 - i think it makes sense to to have this feature because using SkipConstructor() is way more shady then asking users if they want use hidden constructor

  • Usage:
    UseConstructor() to use public parameterless ctor
    UseConstructor(false) to use public parameterless ctor
    UseConstructor(true) to use private/protected parameterless ctor
    UseConstructor(x=>new Instance()) to use custom delegate (CustomInstantiator() marked as obsolete)
    SkipConstructor() to not using constructor at all

When parameterless ctor is not found the NoParameterlessCtorException is thrown

🔧 Found & Fixed bug when StrictMode(true) is used and no RuleFor() is specified
⚠️ .net standard 1.3 is missing few methods so NotSupportedException is thrown istead in this assembly

Sorry for using different code formatted in advanced ... 😃
Also question is it necessery to have T in Faker[T] to be reference type? Because i would consider generating structures handy and looking very briefly i didn't see any reason why it would not be possible



[Fact]
public void UseContructor_After_SkipConstructor()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is debatable if using both UseConstructor and SkipConstructor should be prohibited or not although it does not break anything.

#else
//Not sure wich works better
//this.CreateActions[Default] = (Faker faker) => (T)FormatterServices.GetUninitializedObject(typeof(T));
UseConstructor((Faker faker) => (T)System.Runtime.Serialization.FormatterServices.GetUninitializedObject(typeof(T)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this.CreateActions[Default] should be called or this.CreateActions[currentRuleSet] instead.

{
if( populateActions.TryGetValue(propOrFieldOfT, out var populateAction) )
if (populateActions != null && populateActions.TryGetValue(propOrFieldOfT, out var populateAction))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned bug was here when no RuleFor() was called (Or simple AddRule) when StrictMode(true) the populateActions variable was null and resolved in null refernce exception

@bchavez
Copy link
Owner

bchavez commented Nov 24, 2020

@Basyras Thank you for the PR. I really appreciate it! I'll try to provide my thoughts below.

  • Please try to revert formatting changes. Please only change what is minimally necessary. Reviewing this PR is extremely difficult because of the formatting changes. Code merged into Bogus should be production quality and goes through a strict review process because, ultimately, I have to maintain it.

  • Keep the code focused on the new feature only. That is, the PR should only be adding .SkipConstructor() that uses GetUninitializedObject.

  • If you see a bug, the bug fix should be in a separate PR, not merged with a new feature PR. It's too difficult to track and revert if there is a problem with the bug and fix.

If you can, please break up the PR. There are too many changes in this PR. I will try to dedicate more time for this during the holiday break.

Thanks,
Brian

@Basyras
Copy link
Contributor Author

Basyras commented Nov 24, 2020

@bchavez Shared formatting rules from .editorconfig file were not applied. I can see .editorconfig file in your root folder but i think your formatting rules are not inside. That's the reason why my formatting changes were made. Could you fix that? Should be simple, go to your IDE formatting settings and on top there should be option to generate .editorconfig

@gl1tch
Copy link

gl1tch commented Jul 19, 2022

What ever happened to this PR, I don't seem to see it ever made it into the code ?

@Basyras
Copy link
Contributor Author

Basyras commented Jul 22, 2022

@gl1tch nope

@bchavez bchavez force-pushed the master branch 17 times, most recently from 0e50b51 to 0be1f8e Compare December 22, 2023 18:32
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