RevEng: Split identifiers into words and Pascal-case them. #5360
Conversation
|
Adding |
|
|
|
Checked in with commit a065b93. |
| @@ -157,13 +157,13 @@ FOREIGN KEY (parentid_A, parentid_B) REFERENCES parent (id_a, id_b) | |||
|
|
|||
| var propList = new List<Property> | |||
| { | |||
| (Property)children.FindProperty("ParentId_A"), | |||
| (Property)children.FindProperty("ParentId_B") | |||
| (Property)children.FindProperty("ParentIdA"), | |||
There was a problem hiding this comment.
Sorry I am only chipping in after this has been merged but this string reveals that we weren't all exactly on the same page with the rules 😞.
In my mind one of the rules was that if a string contained mixed case we would assume that the user already had it the way they wanted it to be and would avoid any further transformation, including removing "_"s.
@rowanmiller @roji thoughts on this?
There was a problem hiding this comment.
What we have is consistent with what I thought we'd come up with 😄 😕 . My understanding was that the pascal casing was only skipped on a per-segment basis after we had split on the underscore... so in this case we leave it as "ParentId" rather than making it "Parentid".
There was a problem hiding this comment.
@divega My understanding was the same as @rowanmiller's. But see also 2dc6007 where I checked in an update because I had missed a test that became broken because of this. For that test there is a table called "E F" which becomes an Entity called "E_f" because we split words only on '_' and '-' and so the algorithm regards all 3 chars as one "word" which it Pascalizes to "E f" which then goes through the part of algorithm which makes sure it's valid C# and we end up with "E_f".
I'm not sure how far we want to go down this route as it's a fairly unusual case but maybe we should add in more characters which act as "word separators"?
There was a problem hiding this comment.
I would treat " " the same as "_" and "-".
There was a problem hiding this comment.
What about '+' or '&' or any kind of whitespace? Or punctuation?
There was a problem hiding this comment.
That's probably overkill I think, the common ones I have seen are "-", "_", and " " (though I think underscores are the most common).
There was a problem hiding this comment.
OK. Have re-opened #3987 and will submit another PR for that.
There was a problem hiding this comment.
@lajones @rowanmiller I don't feel super strongly about this but I would like to make sure you can see what I mean: if I have a column in the database called ParentId_A the chances that I expect or want reverse engineering to fiddle with it to produce a property called ParentIdA are almost nil IMO. E.g. if I already knew to use mixed case in ParentId then the underscore is likely there on purpose.
It is just heuristics but I don't think there is a way to turn it off, so I am trying to apply the principle of least surprise.
Fix for #3987.
The code change is fairly small but this is a fairly big change to the names of identifiers for classes and properties generated by scaffolding (and so a large number of expected results files change).
(Also note: I found we had 2 copies of the
CSharpUtilitiesclass - so I removed one of them.)Negatives of this approach: there will need to be more
[ColumnName]and[TableName]attributes (or equivalent fluent API).Positives: tends to make the resulting names look more like C#. We get better (more "C#-looking")identifiers for DB table/column names which contain underscores or hyphens and also better results for camel-cased or all-caps or (some) mixed case.
This will obviously be a matter of taste but we think the positives outweigh the negatives here.