Added support for objects which override the ToString method in SessionModelConverter #327

Merged
merged 5 commits into from Apr 22, 2013

Conversation

Projects
None yet
3 participants
@taylan
Contributor

taylan commented Apr 19, 2013

SessionModelConverter only had support for types marked as Serializable.

With this change, it also checks if the type has overridden the ToString
method in which case it calls it. If the type is neither marked as
serializable nor has the ToString method, then the "Non serializable
type" error message is shown in the Session tab.

Added support for objects which override the ToString method in Sessi…
…onModelConverter

SessionModelConverter only had support for types marked as Serializable.
With this change, it also checks if the type has overridden the ToString
method in which case it calls it. If the type is neither marked as
serializable nor has the ToString method, then the "Non serializable
type" error message is shown in the Session tab.
@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Apr 19, 2013

Member

This looks like a good pull request and idea to me.

@avanderhoorn do you have any thoughts or objections?

There is one last point of business though @vape, we need you to sign our contributor's license agreement and return it before we can take this in.

You can email it to me. I have the same handle at Gmail.

Member

nikmd23 commented Apr 19, 2013

This looks like a good pull request and idea to me.

@avanderhoorn do you have any thoughts or objections?

There is one last point of business though @vape, we need you to sign our contributor's license agreement and return it before we can take this in.

You can email it to me. I have the same handle at Gmail.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Apr 19, 2013

Member

Looks good to me. Only thing that would be nice is a unit test to go in here https://github.com/glimpse/Glimpse/tree/master/source/Glimpse.Test.AspNet/SerializationConverter. But to be fair we don't have one already, it must have been missed. So only if you feel like it, that would be great, but otherwise good to go on my end.

Member

avanderhoorn commented Apr 19, 2013

Looks good to me. Only thing that would be nice is a unit test to go in here https://github.com/glimpse/Glimpse/tree/master/source/Glimpse.Test.AspNet/SerializationConverter. But to be fair we don't have one already, it must have been missed. So only if you feel like it, that would be great, but otherwise good to go on my end.

@taylan

This comment has been minimized.

Show comment
Hide comment
@taylan

taylan Apr 20, 2013

Contributor

Hi Anthony,

I'll add the unit tests and append that commit to this pull request.

I'm going to send the license agreement form as well. You can accept the pull request after I'm done with those.

Contributor

taylan commented Apr 20, 2013

Hi Anthony,

I'll add the unit tests and append that commit to this pull request.

I'm going to send the license agreement form as well. You can accept the pull request after I'm done with those.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Apr 20, 2013

Member

Huge thanks mate, great work!

On Sat, Apr 20, 2013 at 11:36 AM, Taylan Aydinli
notifications@github.comwrote:

Hi Anthony,

I'll add the unit tests and append that commit to this pull request.

I'm going to send the license agreement form as well. You can accept the
pull request after I'm done with those.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/327#issuecomment-16702010
.

Member

avanderhoorn commented Apr 20, 2013

Huge thanks mate, great work!

On Sat, Apr 20, 2013 at 11:36 AM, Taylan Aydinli
notifications@github.comwrote:

Hi Anthony,

I'll add the unit tests and append that commit to this pull request.

I'm going to send the license agreement form as well. You can accept the
pull request after I'm done with those.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/327#issuecomment-16702010
.

@taylan

This comment has been minimized.

Show comment
Hide comment
@taylan

taylan Apr 20, 2013

Contributor

Hi guys,

I added unit tests for the SessionModelConverter.

They all pass but I'm not 100% sure that I wrote them as I should or if there are any missing tests. (I'm still new to the codebase and the results of the SessionModelConverter are of an odd type) I would appreciate it if you check them out and point out if there are any problems. I'll deal with the agreement form now

Contributor

taylan commented Apr 20, 2013

Hi guys,

I added unit tests for the SessionModelConverter.

They all pass but I'm not 100% sure that I wrote them as I should or if there are any missing tests. (I'm still new to the codebase and the results of the SessionModelConverter are of an odd type) I would appreciate it if you check them out and point out if there are any problems. I'll deal with the agreement form now

taylan added some commits Apr 21, 2013

Merge branch 'master' of https://github.com/vape/Glimpse
Conflicts:
	source/Glimpse.Test.AspNet/SerializationConverter/SessionModelConverterShould.cs

avanderhoorn added a commit that referenced this pull request Apr 22, 2013

Merge pull request #327 from vape/master
Added support for objects which override the ToString method in SessionModelConverter

@avanderhoorn avanderhoorn merged commit 739afdb into Glimpse:master Apr 22, 2013

1 check passed

default TeamCity Build Glimpse :: Continuous Integration finished: Tests passed: 945, ignored: 10
Details
@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Apr 22, 2013

Member

Huge thanks mate! If you are interested in building out some more stuff let us know! We have some cool ideas for how we could capture what session looked like at the beginning of the request vs the end.

Member

avanderhoorn commented Apr 22, 2013

Huge thanks mate! If you are interested in building out some more stuff let us know! We have some cool ideas for how we could capture what session looked like at the beginning of the request vs the end.

@taylan

This comment has been minimized.

Show comment
Hide comment
@taylan

taylan Apr 24, 2013

Contributor

Hi Anthony,

I would be interested in working on that feature, or any other that you are planning to implement.

Does anyone have a general idea about how that feature would work, or is it just an idea? I couldn't find any discussions on this in the Google Group.

Contributor

taylan commented Apr 24, 2013

Hi Anthony,

I would be interested in working on that feature, or any other that you are planning to implement.

Does anyone have a general idea about how that feature would work, or is it just an idea? I couldn't find any discussions on this in the Google Group.

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Apr 24, 2013

Member

More of an idea at this point.

Essentially a tab states when it wants to run via the ExecuteOn property. The RuntimeEvent that is returned supports [Flags], so you can return RuntimeEvent.BeginSessionAccess | RuntimeEvent.EndSessionAccess and GetData() will be called twice.

You can use the first call as an opportunity to show the data in session as it was when the request began, and use the second call to detect what has changed.

Member

nikmd23 commented Apr 24, 2013

More of an idea at this point.

Essentially a tab states when it wants to run via the ExecuteOn property. The RuntimeEvent that is returned supports [Flags], so you can return RuntimeEvent.BeginSessionAccess | RuntimeEvent.EndSessionAccess and GetData() will be called twice.

You can use the first call as an opportunity to show the data in session as it was when the request began, and use the second call to detect what has changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment