Skip to content

Conversation

kkozmic
Copy link
Contributor

@kkozmic kkozmic commented Aug 11, 2013

I was planning to clean up/rebuild the reporting pipeline but I'll do that in a separate PR. I won't have time to do this today

Not terribly convinced yet I prefer this approach, but it's an experiment.
This allows us to remove static factory method call and we can, if needed, treat this object as a context object if needed.

Also changed the Execute method on the convention to be void... it technically makes things simpler, although on the other hand, doesn't in any way indicate you're supposed to call result.Something and that you're supposed to do it only once...

We'll just need to play with it and see where it takes us
I quite liked the simplicity of @JakeGinnivan's solution that just returned IEnumerable<object>

This allows us to do that as an option, but also to fallback to using IConventionResult explicitly when we need more control
introduced IConventionContext (to be used internally).
Still a lot of cleaning up left to do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recon get rid of this, I would prefer that we just use result.Is for all conventions and not try to put this in for compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough

@JakeGinnivan
Copy link
Member

It's a bit hard to see all the changes, but have noticed a few things with the formatting.

You always have

public type Prop
{
    get { return ...; }
}

Also, long lambdas are like:

var ex = 
    Assert.Throws<Exception>(
        () => foo());

I think we should agree on this as it will stop unnecessary changes in the PR's. I am happy to always expand the getters and not line it up on a single line.

var ex = Assert.Throws<Exception>(
    () => foo());

Is my preferred wrapping as the first line is still short enough, but it doesn't have that staircase appearance.

Thoughts? (let me know any other styles which you would like to agree on)

@JakeGinnivan
Copy link
Member

@kkozmic so I thought id try and separate a few things, I love the change with the IConventionResult and dropping the ISymmetricConvention!

So I have created #25 which steals that idea, but doesn't make any other changes.

@JakeGinnivan
Copy link
Member

The context approach is interesting. I think it needs to be it's own PR with no other things mixed in

Let me know if #25 is what you were trying to do, because it certainly seemed to make things cleaner

@kkozmic
Copy link
Contributor Author

kkozmic commented Aug 11, 2013

about formatting, I'm letting Resharper handle that.
I would have thought it'd pick up the .dotsettings file and apply that cross-team.

Let me know if it gives you different results.

In particular, it breaks the line at 120 chars (the default) but's nice when viewing the source on github since it shows just over 120 chars per line.

I'm open to discussing changes in the style but perhaps it'd be better to open a separate issue for that.

@kkozmic kkozmic closed this Aug 17, 2013
@kkozmic kkozmic deleted the reporting2 branch August 17, 2013 23:05
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.

2 participants