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

Adds MatchConstructorParametersWithUnderscores #1786

Closed
wants to merge 1 commit into from

Conversation

jo-goro
Copy link
Contributor

@jo-goro jo-goro commented Jun 16, 2022

Resolves #818.

Adds the static MatchConstructorParametersWithUnderscores property for DefaultTypeMap which allows matching columns containing underscores to constructor parameters (eg. user_id matches userId).

This commit also renames MatchNamesWithUnderscores to MatchFieldsAndPropertiesWithUnderscores for clarity. MatchNamesWithUnderscores is marked as obsolete and simply maps to MatchFieldsAndPropertiesWithUnderscores.

@oliverlevay
Copy link

Merge this pleeeaaaasse

@AdamTovatt
Copy link

Please merge this

@b2soft
Copy link

b2soft commented Aug 11, 2023

Would be great if merged, thanks

@mgravell
Copy link
Member

mgravell commented Aug 11, 2023

I have been thinking about this one (due to the bumps), mostly wondering: "do we actually need the extra properties?"

Help me reason here; the current scenario is that if there are underscores and consequently no match, then today: such code would fail. As long as we're only changing "failing code to working code", I don't think we need the granular properties, and the pre-existing MatchNamesWithUnderscores is fine (as long as the intent is respected)

Thoughts?

Is there any scenario in which this could change working code to become "not working" or "works differently" ? I'm not sure there is, as long as exact matches are tested first, and normalized matches tested second.

@mgravell
Copy link
Member

that might mean that the preferred option is for GetConstructorParameter to attempt an exact match first, and then and only then if (MatchNamesWithUnderscores) { /* normalize and retry */ } ?

@b2soft
Copy link

b2soft commented Aug 11, 2023

that might mean that the preferred option is for GetConstructorParameter to attempt an exact match first, and then and only then if (MatchNamesWithUnderscores) { /* normalize and retry */ } ?

For me that looks logical. Leave the same variable name, and just exact match + check underscores if setting is true. But I'm open to other options as well.

@AdamTovatt
Copy link

I just don't want to have to use underscores for the parameter names in my constructors.

If it is implemented as the default behavior without a specific property for just constructors or if it is implemented with a specific property for constructors doesn't matter to me, I think both works.

I think you are saying that it is unnecessary to add the extra property when it could be the default behavior (given the existing MatchNamesWithUnderscores), which I agree with. I just assumed it was written that way to avoid changing the behavior of already existing code. I trust that someone smart will make a smart decision here, as long as I don't have to use snake case in my constructors I'm happy :)

@mgravell
Copy link
Member

Yep; I'll take a look ASAP and see if I can validate the behaviour without this setting; if it is what I think, I'll take on the work to tweak the PR (preserving credit) to remove the extra properties, which I believe we can side step.

@mgravell
Copy link
Member

I've validated to my satisfaction that we can add this without the additional settings, since any scenario affected currently fails as:

System.InvalidOperationException : A parameterless default constructor or one matching signature ({signature}) is required for {typename}

I'll see about tweaking the PR

@AdamTovatt
Copy link

That's nice, it is even better without the additional setting, like you said

@mgravell mgravell mentioned this pull request Aug 17, 2023
@mgravell
Copy link
Member

This PR combined and extended in #1947

@mgravell mgravell closed this Aug 17, 2023
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.

DefaultTypeMap.MatchNamesWithUnderscores is ignored for ctor parameters
5 participants