Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic project setup with unit tests and first classes #1

Closed
wants to merge 6 commits into from
Closed

Basic project setup with unit tests and first classes #1

wants to merge 6 commits into from

Conversation

brianjmiller
Copy link
Member

Just opening this PR to get initial feedback on things such as project setup, repo setup, core class definitions, JSON handling, anything related to Visual Studio and/or the environment. This is my first foray into Microsoft world development so would appreciate early feedback.

On the JSON handling front, we can go the serializer/deserializer route but after extensive trials with the TinCanJava lib development it ended up just being more of a PITA than it was worth and we ultimately decided on this more manual of approaches there. So, to make them match as close as possible I've continued that approach here. We can re-raise the issue if someone feels strongly that it is the wrong approach. Hopefully other than the JObject constructors and toJObject methods the rest of it is sufficiently encapsulated such to not matter.

@brianjmiller
Copy link
Member Author

Particularly interested in feedback from @bscSCORM @tseabrooks @mlefavor @oconnetf @ingramj .

{
interface JSON
{
JObject toJObject(TCAPIVersion version);
Copy link
Member

Choose a reason for hiding this comment

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

Since we only have to support C# here, let's clean this up by using default parameters. eg:

JObject toObject(TCAPIVersion version = TCAPIVersion.latest());
toJSON(TCAPIVersion version = TCAPIVersion.latest(), bool pretty = false);

@brianjmiller
Copy link
Member Author

@bscSCORM and I took this discussion off line and I've gotten about as close to requested changes as we could determine without going down very long trail that we generally want to avoid. Any other feedback appreciated.

@brianjmiller brianjmiller self-assigned this Apr 10, 2014
* Includes LRSResponse base implementation
* Includes About and Extensions model handling
* Improvements to Version handling to provide for getting
  supported versions and getting a version given a string
* Corrections to map handling and .isEmpty method
* Add try/catch to /about call for web exception
* Add property to LRSResponse.Base for storing exception
* Add stubs for passing additional headers and query params to GetRequest
@hardlyknowem
Copy link

This is more a question for my own education than anything else: Why the preference for using JObjects, rather than just letting Json.NET do it's regular magic serialization/deserialization based on fields and properties? Is it a performance concern? Design concern?

@brianjmiller
Copy link
Member Author

See original comment in PR. Mostly, after going back and forth when building the TinCanJava lib it was more pain than it was worth because of the need to support multi-version.

@hardlyknowem
Copy link

D'oh. I can't believe I missed that comment. I will say that I found Json.NET's handling of subclass dispatch (which I assume was the trouble you had with multi-version) to be much easier to configure than Jackson's (which I'm assuming you used for Java). But keeping the design close to the way Java was designed is probably more important.

* Adds overall structure to handle synchronous HTTP requests
@brianjmiller
Copy link
Member Author

Closing because I'm rewriting history to correct author info. Will re-open for actual merge.

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.

3 participants