IExpressionToCode with rules for FullTypeNames and fixed issue #13 #42

dadhi · 2015-02-08T13:23:11Z

Compared to previous PR: fixed full generic type name.

… of ToCode expression methods. added: Rules for specifying FullTypeNames and ExplicitMethodTypeArgs. fixed: #13 Explicit type parameters for methods omitted - Instead of inferring which is complex task, I want to have setting to turn it on for my use case, and be off by default.

added: Propagation of FullTypeNames option from ExpressionToCodeImpl to ObjectToCode. changed: Made CSharpFriendlyTypeName public to be used in generation scenarios alongside with ToCode.

…count.

EamonNerbonne · 2015-02-10T09:23:07Z

While I think this is a good, I have a mild preference to avoid new public API until the dust has settled; and I'm sure this isn't going to be the only rule that needs parameterization.

Options:

We just add public API anyhow - accepting that later API cleanup may be painful.
We add new public API, but in an "Internal" namespace that's documented to be unstable, and clean it up there.
We do this in a non-master branch, aiming for ExpressionToCode v2.

I think I've got a preference for option 2 - that way it's usable if you're in an experimental mood, but automatic upgrades aren't broken yet. I think this would also be a good place to integrate several of @asd-and-Rizzo suggestions, who noticed inconsistencies in naming, such as the namespace ExpressionToCodeLib.

EamonNerbonne · 2015-02-10T09:23:58Z

ExpressionToCodeLib/CSharpFriendlyTypeName.cs

@@ -4,8 +4,13 @@
 using System.Text;

 namespace ExpressionToCodeLib {
-    static class CSharpFriendlyTypeName {
-        public static string Get(Type type) { return GenericTypeName(type) ?? ArrayTypeName(type) ?? AliasName(type) ?? NormalName(type); }
+    public static class CSharpFriendlyTypeName {


This doesn't need to be public, right?

No, I made it public because I need it in my gen scenario: dictionary of Type -≥ Expression Code. So I need Type literal. I have similar utility method in DryIoc, but yours covers more cases.

Or, suddenly! Could I use ObjectToCode? If so, then it is fine to stay internal.

You can use ObjectToCode.

EamonNerbonne · 2015-02-15T11:12:49Z

Right, there are still a few fairly small points - can you address those? I'd love to get this feature merged.

Incidentally, if you want, I can give you commit access to this repo directly. I still like the PR model, but that can make further development easier, and some small refactorings don't work well in a PR (mass renames, trivial inlines; things like the CSharpFriendlyTypeName.Get => GetCSharpFriendlyTypeName change.

If this is all too much work; I get that too - thanks for the great contributions so far!

dadhi · 2015-02-16T07:35:25Z

Let's summarize the PR issues so far:

Make CSharpFriendlyTypeName internal again. Fine with me
Move Func<Type, string> GetCSharpFriendlyTypeName to Rules.
I think it need more thought to it.. May be make ObjectToCode implement IObjectToCode, and then make ExpressionToCode depend on interface. Not sure yet, but I don't like current hidden dependency - especially if we introduce rules/policies scattered in both Object\ExpressionToCode.
Rules itself:
- Implement with struct or MemberwiseClone? Or stay with current approach?
- Change construction/application approach? Or stay with current approach?

My plan is to look into code this week, and try to address those.

EamonNerbonne · 2015-02-16T14:27:41Z

On GetCSharpFriendlyTypeName: the point is that you don't even need a delegate; a plain method is sufficient. There's no advantage I can think of to performing the if on the rules early; and the downside to delegates is that the code is less traceable (things like go to definition become less useful).

However, this is a minor point, not blocking for a merge.

dadhi · 2015-02-16T17:23:21Z

Oh, I see. Agree.
I probably pull your latest changes, merge, fix issues and submit new PR.

added: ObjectToCode.Default and ObjectToCode.WithFullTypeNames predefined instances. YAGNI for now. changed: Inlined Rules into ExpressionToCode - too much overhead for such a simple setup. changed: Made ExpressionToCode depend on IObjectToCode implementation. Merge branch 'master' of https://github.com/EamonNerbonne/ExpressionToCode

dadhi · 2015-02-17T12:27:56Z

Hi,
Sorry I put merge and fixes into the same commit.
Checking it, seems nothing have lost from your changes.
Up to you to merge.

dadhi · 2015-03-04T09:36:08Z

Hello, may be it is easy to provide me commit rights?
If you OK with where I am going with my changes.

IExpressionToCode with rules for FullTypeNames and fixed issue #13

EamonNerbonne · 2015-03-05T21:46:20Z

Hi Maksim,

I added you to the contributors list & merged this PR - I'm definitely happy with where it's going :-).

However, there are a few issues with these changes currently that make releasing this version to nuget less that ideal:

this PR changes the public API quite a bit. I added an approval test to highlight the changed API. My personal preference would be to build the new (instance method) API in the unstable namespace, and implement the old static methods on top of that. That way we can refactor and experiment with the unstable api, and keep on releasing bugfixes simultaneously without impacting dependant projects. I expect that this new "configurable" API will change quite a bit since there's likely to be more than this that would be nice to configure. This isn't strictly a blocking issue, but it'd make life easier.
Friendly type names for unbound generics cause a stack overflow (regression). I added a test for that too. This needs to be fixed before the next nuget release. While adding tests for this, I noticed that the old behavior wasn't perfect either; but it certainly shouldn't crash.

Sorry for the ridiculously long delay!

dadhi added 3 commits February 4, 2015 15:56

fixed: Nested Type names with FullTypeNames option.

72f8cf5

added: Propagation of FullTypeNames option from ExpressionToCodeImpl to ObjectToCode. changed: Made CSharpFriendlyTypeName public to be used in generation scenarios alongside with ToCode.

fixed: System.FormatException when Parsing GenericTypeName arguments …

a05ca84

…count.

EamonNerbonne reviewed Feb 10, 2015
View reviewed changes

EamonNerbonne added a commit that referenced this pull request Mar 5, 2015

Merge pull request #42 from dadhi/master

e835fef

IExpressionToCode with rules for FullTypeNames and fixed issue #13

EamonNerbonne merged commit e835fef into EamonNerbonne:master Mar 5, 2015

EamonNerbonne added a commit that referenced this pull request Mar 7, 2015

Re-enable tests to prepare to fix remaining issues with #42.

cf974d7

This was referenced Mar 7, 2015

Fix stackoverflow error in stringification of generic type arguments. #45

Merged

Add options parameter to generate full namespaces. #29

Closed

EamonNerbonne mentioned this pull request Aug 26, 2022

Option disable literal strings #245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

IExpressionToCode with rules for FullTypeNames and fixed issue #13 #42

IExpressionToCode with rules for FullTypeNames and fixed issue #13 #42

Uh oh!

dadhi commented Feb 8, 2015

Uh oh!

EamonNerbonne commented Feb 10, 2015

Uh oh!

EamonNerbonne Feb 10, 2015

Uh oh!

dadhi Feb 10, 2015

Uh oh!

dadhi Feb 10, 2015

Uh oh!

EamonNerbonne Feb 15, 2015

Uh oh!

EamonNerbonne commented Feb 15, 2015

Uh oh!

dadhi commented Feb 16, 2015

Uh oh!

EamonNerbonne commented Feb 16, 2015

Uh oh!

dadhi commented Feb 16, 2015

Uh oh!

dadhi commented Feb 17, 2015

Uh oh!

dadhi commented Mar 4, 2015

Uh oh!

EamonNerbonne commented Mar 5, 2015

Uh oh!

Uh oh!

IExpressionToCode with rules for FullTypeNames and fixed issue #13 #42

IExpressionToCode with rules for FullTypeNames and fixed issue #13 #42

Uh oh!

Conversation

dadhi commented Feb 8, 2015

Uh oh!

EamonNerbonne commented Feb 10, 2015

Uh oh!

EamonNerbonne Feb 10, 2015

Choose a reason for hiding this comment

Uh oh!

dadhi Feb 10, 2015

Choose a reason for hiding this comment

Uh oh!

dadhi Feb 10, 2015

Choose a reason for hiding this comment

Uh oh!

EamonNerbonne Feb 15, 2015

Choose a reason for hiding this comment

Uh oh!

EamonNerbonne commented Feb 15, 2015

Uh oh!

dadhi commented Feb 16, 2015

Uh oh!

EamonNerbonne commented Feb 16, 2015

Uh oh!

dadhi commented Feb 16, 2015

Uh oh!

dadhi commented Feb 17, 2015

Uh oh!

dadhi commented Mar 4, 2015

Uh oh!

EamonNerbonne commented Mar 5, 2015

Uh oh!

Uh oh!