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

Refactor to get rid of duplicate TableInfo/ColumnInfo code #530

Merged

Conversation

Projects
None yet
2 participants
@asherber
Copy link
Collaborator

commented May 11, 2019

@pleb Interested in your thoughts on this. I noticed that while StandardMapper calls methods in ColumnInfo and TableInfo to get information, ConventionMapper duplicates that code and then adds a little extra processing (like inflection). I thought it would be good to refactor so that ConventionMapper can call the same base methods in ColumnInfo and TableInfo and then do what it needs to.

(One practical implication of the duplication is that when I added IncludeInAutoSelect to ResultColumnAttribute, I only handled it it ConventionMapper, because I didn't see the other methods. Now it's handled in ColumnInfo and picked up by both ConventionMapper and StandardMapper.)

The refactored methods are not as clean as I would like them – they've got a couple of out params so that ConventionMapper can avoid a second round of reflection. On the whole, that's probably an okay tradeoff.

@pleb

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I'll have a better look tonight. But, from your description and logical conclusion, I'd say you've likely picked the best route. There are a few parts which could do with a big refactor tidy, namely the mapping (this) and ILGen mapper generator.

@pleb

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Sorry, I keep getting help up at night.

I've had a read through, and everything seems to line up.

@pleb pleb merged commit d0a08ba into CollaboratingPlatypus:development May 14, 2019

1 check failed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.