Provide a way to limit the chars to store in each column. #499
Conversation
|
@sebastienros please approve running this PR so I can see the tests results.
|
|
@sebastienros this sucks. With every commit someone has to approve the run :) If there a way to avoid this? meanwhile, can you please approve it one more time? |
| /// </summary> | ||
| /// <param name="name"></param> | ||
| /// <returns></returns> | ||
| protected virtual string GetRawColumnName(string name) |
There was a problem hiding this comment.
I would prefer to see the Run method rewritten using a local function. GetRawColumnName doesn't make sense on the base class. And the name doesn't even match what it's used for.
There was a problem hiding this comment.
@sebastienros I override the run function. Please approve the PR Pipeline to run so we can see the result.
|
@sebastienros it looks like all tests passed. If you are good with this, you may want to merge it and do a quick release. I can then take it from there and apply it in OC so we can ship OC 1.7.1 soon (hopefully during triage) |
|
@sebastienros I can always merge this, and release 3.3.1 which should push the new release to nuget. Would you like me to do that? Or do you want to wait or do it yourself? |
| ); | ||
| } | ||
|
|
||
| private static readonly Regex ColumnNamePattern = new(@"([a-zA-Z_][a-zA-Z0-9_]+)\s*(\(\s*(\d+)\s*\))?"); |
There was a problem hiding this comment.
Just split the string with '(', ')', ' ' with the RemoveEmptyEntries options, and take the second result. I think Regexes are overkill here (and prone to issues).
There was a problem hiding this comment.
@sebastienros done.
Let me know about merging and releasing
|
|
||
| private string GetColumName(string name) | ||
| { | ||
| var parts = name.Split(new[] { '(', ')', ' ' }, StringSplitOptions.RemoveEmptyEntries); |
There was a problem hiding this comment.
It would be better to store the separators into a static field
Fix for the issue exampled here: OrchardCMS/OrchardCore#14338
running the PR pipeline to see if tests will pass.