Contributing changes to GW2.NET

Developer
Dec 27, 2013 at 2:16 PM
Hey guys

How would I go about sending in patches after I make a change to the source code?

It appears that there is no way for me to log on to source control for this project without being a member of the project team. All I could find is an "Upload patch" option on the "Source Code" tab, but I have no idea what to upload there.
Editor
Dec 27, 2013 at 3:31 PM
Hey Steven, Ruhr will need to add you to the project. Once he does that, you should see a Connect option on the Source Code page.
Developer
Dec 27, 2013 at 3:41 PM
Excellent, thanks!

Can I go ahead and assume that everything on the "GW2.NET-Dev" branch is current?
Developer
Dec 28, 2013 at 5:26 PM
Edited Dec 28, 2013 at 6:24 PM
I've been digging through the project files, and I think I have a pretty solid idea of how the code is structured. Now I'm thinking it would be better to get rid of the ApiCall class, since it's basically a singleton.

My idea is to replace ApiCall with sub-classes of RestClient, one class per API category, with specific methods targeting the GW2 API.

Categories are as listed on the wiki:
  • Dynamic events
  • Guilds
  • Items
  • Map information
  • World vs. World
  • Miscellaneous
So you'd get six different clients with specific methods for each category. An example of the MiscellaneousClient would look like this:
    /// <summary>
    /// Contains methods to retrieve API data from the "Miscellaneous" category.
    /// </summary>
    public class MiscellaneousClient : RestClient
    {

        public MiscellaneousClient() : base(Resources.MiscellaneousInformation.BaseUrl) { }


        /// <summary>
        /// Gets the current build id of the game.
        /// This can be used to for example register when event timers reset due to server restarts.
        /// </summary>
        /// <remarks>http://wiki.guildwars2.com/wiki/API:1/build</remarks>
        public BuildInformation GetBuild()
        {
            Uri resource = new Uri(Resources.MiscellaneousInformation.Build, UriKind.Relative);
            IRestRequest request = new RestRequest(resource, Method.GET);
            request.AddHeader("Accept", "application/json");
            IRestResponse response = Execute(request);
            return JsonConvert.DeserializeObject<BuildInformation>(response.Content);
        }

        /// <summary>
        /// Gets all dyes in the game, including localized names and their color component information.
        /// </summary>
        /// <remarks>http://wiki.guildwars2.com/wiki/API:1/colors</remarks>
        public object GetColors()
        {
            // TODO
            throw new NotImplementedException();
        }


        /// <summary>
        /// Gets commonly requested in-game assets that may be used to enhance API-derived applications.
        /// The returned information can be used with the render service to retrieve assets.
        /// </summary>
        /// <remarks>http://wiki.guildwars2.com/wiki/API:1/files</remarks>
        public object GetFiles()
        {
            // TODO
            throw new NotImplementedException();
        }

    }
Now since this is actually a pretty huge change, I'd like to get your input on this. Every procedure that targets ApiCall would need refactoring. And really the only benefit would be making the code more maintainable in the event of API changes.

EDIT

I saw some discussion about whether to go with RestSharp or Microsoft's HttpClient package. Why would you have to choose?

Looking at the code example that I wrote, I can easily imagine myself extracting a common interface, then implementing it in a client class for both frameworks. Choosing which client should provide the API data now becomes a decision for the implementer.
Editor
Dec 28, 2013 at 8:16 PM
Yes, typically -Dev is the most recent, though I haven't actually looked at this code in a few months. Ruhr did some recent work on it, so hopefully he'll chime in soon and get you added. He's probably on holiday or something.

Sounds like you're full of good ideas. I don't see a problem with implementing any of your suggestions.

Also, I wonder if Ruhr would be interested in switching this project to use git source control. The wiki says if you contact the Codeplex folks, they'll convert it for you (https://codeplex.codeplex.com/wikipage?title=CodePlex%20FAQ#TFStoMercurial). It's just a vastly better system for open source projects, because it makes it a lot easier for new contributors to start playing with the code without having to wait for a project coordinator who is on holiday. :)
Developer
Dec 28, 2013 at 11:27 PM
Edited Dec 28, 2013 at 11:33 PM
bilong wrote:
Yes, typically -Dev is the most recent, though I haven't actually looked at this code in a few months. Ruhr did some recent work on it, so hopefully he'll chime in soon and get you added. He's probably on holiday or something.

Sounds like you're full of good ideas. I don't see a problem with implementing any of your suggestions.
What I'm proposing is really not all that different from the code that's already been written. It's just the static ApiCall class that's been giving me a sour taste in my mouth.

From what I gather, a data provider that references ApiCall.GetContent() is actually a funny implementation of a decorator pattern. Essentially, each data provider wraps a RestClient, decorating it with methods to convert the API data to concrete objects.

You can almost say that each one of the data providers is also a RestClient. It's just not immediately obvious. Not until you look at a class diagram.

Code Map

bilong wrote:
Also, I wonder if Ruhr would be interested in switching this project to use git source control. The wiki says if you contact the Codeplex folks, they'll convert it for you (https://codeplex.codeplex.com/wikipage?title=CodePlex%20FAQ#TFStoMercurial). It's just a vastly better system for open source projects, because it makes it a lot easier for new contributors to start playing with the code without having to wait for a project coordinator who is on holiday. :)
I'd love that. Right now, I'm using a copy of the source code that I extracted from a zip file. I did find out today that I do have read-only Subversion access, but I mean really...?
Coordinator
Jan 1, 2014 at 6:39 PM
Edited Jan 1, 2014 at 6:42 PM
Yeah holidays, there I was :P I'll add you to the list of contributors right away.

The latest branch is not the -dev one, The latest is the main branch. -Dev was used for testing out something new.

As for sour proposals: Sounds good to me. However: I'm currently in the process of reordering some parts of the AI in the async branch. As MS released the new async/await keywords for .Net 4.0 I'm currently in the process to rewrite parts of the api to use those keywords as they make the code much more maintainable. I also tried to implement a non-static vlass used for API calls. It should be in the -asnyc branch, but I'm not sure. If they are not, I'll upload them quickly.

Reordering became necessary, since ANet added a new function to the api, the map information part. Until now we had all information of the maps, and floors in the events itself. This becomes cumbersome for the user. What if I just want to know if an event is in a particular state, then I don't want to know on which floor the event is etc. The current implementation makes a call necessary.
Therefore I changed the event model in such a way, that only the event part of the api is queried. I'm currently in the middle of rewriting those parts, unfortunately the holidays came and I couldn't code in the last few days. I hope to get it done until next weekend.
This means we are going to have
  • Dynamic events
  • Guilds
  • Items
  • Map information
  • World vs. World
  • Miscellaneous
as data providers, each with the details provided here
I'd like you to elaborate the changes a bit more, so I can make up my mind. For now I'm pleased with the suggestions.

As for git. I'm not a hug fan of Git. Primarily because VS had not a good git implementation. 2013 apparently changed that so I will look into this in the next few days. If there is a benefit we can switch.
Editor
Jan 1, 2014 at 7:54 PM
The Git interface is better in VS2013, but if you really want a good Git GUI, download GitHub For Windows. It's a nice, clean interface, and works with any Git provider, including Codeplex. Installing GitHub For Windows also creates a Git Shell shortcut, which takes you to a Powershell prompt with Posh Git loaded.
Coordinator
Jan 1, 2014 at 8:13 PM
Yeah that's the thing I like to do my checkouts from VS. I'll look into it, definitely!
Developer
Jan 1, 2014 at 9:32 PM
Edited Jan 1, 2014 at 10:01 PM
Ruhrpottpatriot wrote:
Yeah holidays, there I was :P I'll add you to the list of contributors right away.
Fantastic, thanks!

Ruhrpottpatriot wrote:
...as data providers, each with the details provided here
I'd like you to elaborate the changes a bit more, so I can make up my mind. For now I'm pleased with the suggestions.
I've thought about it some more. I think the best way to go is with a command pattern, so that every API endpoint (and all of its possible parameters) is encapsulated in its own ApiCall class. If I do it that way, there would only be a single client class that handles these request objects and deserializes their response.

The only problem that I can think of is how to map JSON responses to concrete classes without introducing some form of class coupling between the request and response classes. I suppose you could figure that out by comparing the request URI, but there's probably a better way to do it.

EDIT

My first idea is pretty much exactly the same as the one you describe. The thing that bothered me is that the API endpoints themselves are not divided into categories. In fact, only WvW endpoints are located on a different path. It doesn't make much sense to me to start separating endpoints only because they represent different aspects of the game.


Ruhrpottpatriot wrote:
As for git. I'm not a hug fan of Git. Primarily because VS had not a good git implementation. 2013 apparently changed that so I will look into this in the next few days. If there is a benefit we can switch.
That's okay. I'm new to TFS, but I know that Git can be pretty difficult too.
Coordinator
Jan 1, 2014 at 10:04 PM
I think JSON.NET offers a possibility for that. I'll look through the documentation after I finished the async rework for the events api node.
My way for a "rewrite" of the ApiCall class was to make it not static. Then each data provider creates a new instance of the ANetApiCall class and adds the required parameters via methods. Something like this:

The calling method:
/// <summary>The get event names.</summary>
        /// <returns>The <see cref="Dictionary{Guid, String}"/>.</returns>
        private Dictionary<Guid, string> GetEventNames()
        {
            ANetApiRequest apiRequest = new ANetApiRequest();

            apiRequest.AddParameter("lang", this.apiManger.Language);

            List<Dictionary<string, string>> namesResponse = apiRequest.GetContent<List<Dictionary<string, string>>>("event_names.json", ANetApiRequest.Categories.DynamicEvents);

            // Create a new Dictionary to hold the names,
            // whereas the key is the event id and the values the event name.
            Dictionary<Guid, string> cacheData = new Dictionary<Guid, string>();

            // Iterate through the namesResponse,
            // so we can throw away that damn List<Dictionary<string,string>>! *blargh*
            foreach (Dictionary<string, string> eventName in namesResponse)
            {
                Guid id = new Guid();

                string name = string.Empty;

                foreach (KeyValuePair<string, string> variable in eventName)
                {
                    if (variable.Key == "id")
                    {
                        id = new Guid(variable.Value);
                    }
                    else
                    {
                        name = variable.Value;
                    }
                }

                cacheData.Add(id, name);
            }

            return cacheData;
        }
The ANetApiCall class
 /// <summary>The a net api request.</summary>
    public class ANetApiRequest
    {
        // --------------------------------------------------------------------------------------------------------------------
        // Fields
        // --------------------------------------------------------------------------------------------------------------------

        /// <summary>True when the instance is disposed.</summary>
        private bool disposed = false;

        /// <summary>The arguments dictionary.</summary>
        private Dictionary<string, object> arguments;

        /// <summary>The api category.</summary>
        private Categories apiCategory;

        // --------------------------------------------------------------------------------------------------------------------
        // Constructors
        //// --------------------------------------------------------------------------------------------------------------------

        /// <summary>Initializes a new instance of the <see cref="ANetApiRequest"/> class.</summary>
        public ANetApiRequest()
            : this(new Dictionary<string, object>())
        {
        }

        /// <summary>Initializes a new instance of the <see cref="ANetApiRequest"/> class.</summary>
        /// <param name="args">The arguments for the api request.</param>
        public ANetApiRequest(Dictionary<string, object> args)
        {
            this.arguments = args;
        }

        // --------------------------------------------------------------------------------------------------------------------
        // Enumerations
        // --------------------------------------------------------------------------------------------------------------------

        /// <summary>
        /// Enumerates the possible categories a request can be.
        /// </summary>
        public enum Categories
        {
            /// <summary>
            /// The world part of the API.
            /// Includes world names, map names and events
            /// </summary>
            DynamicEvents,

            /// <summary>
            /// The map information part of the API
            /// </summary>
            MapInformation,

            /// <summary>
            /// The world versus world part of the API.
            /// </summary>
            WvW,

            /// <summary>
            /// The items part of the API
            /// </summary>
            Items,

            /// <summary>
            /// The guild part of the API.
            /// </summary>
            Guilds,

            /// <summary>
            /// The miscellaneous part of the api.
            /// </summary>
            Miscellaneous
        }

        // --------------------------------------------------------------------------------------------------------------------
        // Properties
        // --------------------------------------------------------------------------------------------------------------------

        /// <summary>Gets the category.</summary>
        public Categories Category
        {
            get
            {
                return this.apiCategory;
            }
        }

        /// <summary>Gets the arguments.</summary>
        public Dictionary<string, object> Arguments
        {
            get
            {
                return this.arguments;
            }
        }

        // --------------------------------------------------------------------------------------------------------------------
        // Methods
        // --------------------------------------------------------------------------------------------------------------------

        /// <summary>Adds an argument to the arguments collection.</summary>
        /// <param name="name">The name of the argument.</param>
        /// <param name="value">The value of the argument.</param>
        /// <returns>The <see cref="ANetApiRequest"/>.</returns>
        public ANetApiRequest AddParameter(string name, object value)
        {
            this.arguments.Add(name, value);

            return this;
        }
      
        // Something more, you get the gist.
}
With this, we get rid of the singleton, which is – if I understood you correctly – your biggest concern. We also could extract an interface out of the new ANetApiCall class (which I had in mind), and therefore reduce coupling.
That would be my take on to the problem.
Developer
Jan 1, 2014 at 10:50 PM
Edited Jan 1, 2014 at 11:36 PM
Here's a little class diagram that I pieced together to clarify what I have in mind:

Command Pattern

I've left the sub-classes empty, but obviously this is where the API endpoint as well as additional parameters would be configured.

Once configured, you would then call the GetResponse() method, passing in an instance of RestClient that should handle the network interaction. If successful, the procedure would continue to parse the JSON response and return an object instance that derives from Gw2Response.

All of this can then be wrapped in a dataprovider, but it doesn't need to be. This is wrapping at its simplest, without further abstractions such as caching.

Ruhrpottpatriot wrote:
I think JSON.NET offers a possibility for that. I'll look through the documentation after I finished the async rework for the events api node.
My way for a "rewrite" of the ApiCall class was to make it not static. Then each data provider creates a new instance of the ANetApiCall class and adds the required parameters via methods. Something like this:

The calling method:
...
Pretty good so far. The ideas that we're describing are fundamentally the same. I do get a strong feeling that you're (unintentionally) writing your own version of RestRequest? Keeping a dictionary of arguments instead of setting them directly on a request object seems silly.

Ruhrpottpatriot wrote:
       public enum Categories
       {

       }
I think it's best if we get rid of this enum entirely. I know that you use categories to figure out which base URL to use, but that's not how the API was actually designed. Only the API documentation is grouped based on what endpoints are (somewhat) related to eachother.

I suggest dividing every possible call into a request class based on the endpoint URL (see class diagram). You can later group "related" request/response-pairs in manager classes (EventManager, MapManager, ...), like you did with data providers in current releases.

Ruhrpottpatriot wrote:
With this, we get rid of the singleton, which is – if I understood you correctly – your biggest concern. We also could extract an interface out of the new ANetApiCall class (which I had in mind), and therefore reduce coupling.
That would be my take on to the problem.
Great! I don't mind singletons if they aren't an important part of the project. But in this case, everything else in the library depends on it working properly.
Coordinator
Jan 3, 2014 at 8:13 PM
The enum is a leftover from the static api class. I wanted to rewrite it as non-static first, and then improve it (i.e. removing the enum). I think I'll do that a little bit sooner now.
Developer
Jan 4, 2014 at 1:30 PM
I wouldn't mind taking care of it if your focus is elsewhere. Obviously I'd have to do it my way, but I have a pretty good sense of where you want to go with this and I promise you'll still be able to.
Coordinator
Jan 4, 2014 at 1:37 PM
OK, then: Create a new branch from the mainline and start working on it. I'll rewrite the async branch to use the static implementation for now, so I can switch it later.
Could you create the interface and post it here, so I can change the static implementation so we simply can swap out the implementation?
Developer
Jan 5, 2014 at 8:14 PM
Okay, but first, here's a little example of the workflow that I had in mind. This example retrieves information about a guild:
            // Current API version is 1
            Version apiVersion = new Version(1, 0);

            // Create a new client that targets the specified version
            ApiClient client = ApiClient.Create(apiVersion);

            // Create a request object for retrieving guild information by its name
            ApiRequest guildRequestByName = new GuildRequest("Veterans Of Lions Arch");

            // ... or by its ID
            ApiRequest guildRequestById = new GuildRequest(new Guid("75FD83CF-0C45-4834-BC4C-097F93A487AF"));

            // Send the request and store the response
            ApiResponse response = client.Send(guildRequestByName);

            // Also works asynchronously
            ApiResponse responseAsync = await client.SendAsync(guildRequestById);

            // Finally, deserialize the JSON content
            Guild guildToReturn = response.DeserializeObject<Guild>();
Can you work with something like that? I've only written a small portion of all the classes that would be necessary to support this model. Nothing is set in stone yet.
Coordinator
Jan 7, 2014 at 12:40 PM
Seems good to me, go ahead.
Developer
Jan 7, 2014 at 9:15 PM
Edited Jan 7, 2014 at 10:48 PM
Extracting abstract interfaces was a bit more problematic than I thought it would be.

It should be fairly easy to implement my interfaces in different HTTP libraries (most importantly: RestSharp and HttpClient). However, it's not possible to mix objects from different implementations, even though they have a compatible interface.

For example: it would be impossible to send an API request of type HttpRequestMessage to an API client based on RestClient. After all, RestSharp can only handle its own RestRequest.

So with that in mind, I would appreciate it if you could take a look at my code before I continue. If I can ever figure out how to submit my code using TFS...

EDIT

I managed to create a new branch "GW2.NET-Http". Now I just need to figure out how to add my code to it and submit.

EDIT

Okay, I did it.

FYI: I decided to move the RestSharp-specific implementation to a separate project. This way, the main project will no longer have a RestSharp dependency, leaving the choice of which HTTP library to use up to the implementer. However this also means that your data provider classes must also move to a separate project. Otherwise you'd get a circular dependency.
Coordinator
Jan 8, 2014 at 11:23 AM
Edited Jan 8, 2014 at 11:27 AM
I did a quick glance over the code and it looks solid, but I need more time with it to get an accurate reading (an't do this in time between lectures). However I'd leave the implemetation in the main project for now. I think we should rewrite the server request first and after we have done that we can utilize MEF (or a similar CAF) to make the project modular (with a CAF we'd also don't have to worry about circular dependencies anymore). But I'd move that as the last step, when we have the current API implementation complete.

/edit: One favour I have to ask of you: Could you please use Stylecop according to THIS? It makes reading foreign code so much easier, thanks.
Developer
Jan 8, 2014 at 12:58 PM
Ruhrpottpatriot wrote:
I think we should rewrite the server request first and after we have done that we can utilize MEF (or a similar CAF) to make the project modular (with a CAF we'd also don't have to worry about circular dependencies anymore).
I don't have a clue what that means, but it sounds fascinating. Is MEF something I can learn how to use over the course of a weekend?

Ruhrpottpatriot wrote:
But I'd move that as the last step, when we have the current API implementation complete.
That's okay. I'm not in a hurry.

Ruhrpottpatriot wrote:
/edit: One favour I have to ask of you: Could you please use Stylecop according to THIS? It makes reading foreign code so much easier, thanks.
Sure, no problem. Also I'll add XML comments to my code when I get home.
Coordinator
Jan 8, 2014 at 3:16 PM
Edited Jan 8, 2014 at 3:16 PM
Sadly MEF has a rather steep learning curve. It's rather simple at the beginning, but gets a bit complicated when you have bigger programs. Nonetheless it's a very powerful framework. MEF, or the Managed Extensibility Framework is a framework built into .NET to help programmers to code their applications modular with more ease.

Basically what you do is, you create a Catalog of modules, or parts as they are called in MEF, which can be loaded (and discovered) on demand. It reduces hard coupling between components. To apply it to our project (taken to the extreme):
We would have a core library and every DataManager (i.e Guilds, DynamicEvents) to the api would be in it's own dll, as would be the classes used for calling the api itself (i.e HttpRequest, RestRequest). The API manager class then would discover the parts when they are needed and load them. The most obvious advantage would be that you could patch, say the guilds dll, without touching, and therefore taking down the other parts of the library. A big bonus especially in the server department.

I recommend you read the link at the top to get more informations.

As I said, moving parts of the project into separate dlls it not necessary at this point. However at one point we should think about it, but not now.
Sure, no problem. Also I'll add XML comments to my code when I get home.
Great!
Developer
Jan 8, 2014 at 5:02 PM
Edited Jan 8, 2014 at 5:06 PM
Ruhrpottpatriot wrote:
/edit: One favour I have to ask of you: Could you please use Stylecop according to THIS? It makes reading foreign code so much easier, thanks.
I'm still getting 502 warnings after disabling rule SA1200. Most of them related to "incorrect" word spelling and multiple blank lines in a row. Am I doing something wrong?

EDIT

502 warnings across the entire solution, that is.
Coordinator
Jan 8, 2014 at 5:58 PM
Yeah, I'll fix the Stylecop warnings in the mainline as soon as I can. Resharper and it's Stylecop plugin will tell you if the file is OK
Developer
Jan 8, 2014 at 7:24 PM
Edited Jan 8, 2014 at 9:03 PM
Well... this sure is tedious work. I don't have a Resharper license either. :(

EDIT

Phew! All done.
Coordinator
Jan 9, 2014 at 9:34 AM
Yeah you don't need ReSharper, it just helps a lost. I applied our project for an open source license, maybe we'l get one. This is one of the reasons why I try to get the documentation done as soon as possible, because I know that I'm too lazy to do it afterwards.
Developer
Jan 10, 2014 at 5:03 PM
What are the guidelines for unit testing? I'd like to write automated tests for my code, but I don't know what I'm doing.
Developer
Jan 11, 2014 at 1:50 PM
I ran into a problem with RestSharp today.

I'd like to be able to make concrete response classes that implements IRestResponse, one class for each API endpoint. The API client would then return one of these responses depending on the response URI.

The problem is that Restclient.Execute() is hard-coded to always return a RestResponse, despite having only the interface as the return type.
    public partial class RestClient : IRestClient
    {
        private IRestResponse Execute(IRestRequest request, string httpMethod, Func<IHttp, string, HttpResponse> getResponse)
        {
            IRestResponse response = new RestResponse();
            // ...
            // ...
            // ...
            return response;
        }
    }
This saddens me deeply, since there's no way to override this behavior other than changing their source code.

Anyway, I'll figure out a different solution. I just wanted to point this out.
Developer
Jan 11, 2014 at 9:38 PM
This discussion has been copied to a work item. Click here to go to the work item and continue the discussion.
Developer
Jan 12, 2014 at 5:29 PM
Edited Jan 15, 2014 at 9:28 AM
Okay, I know I'm already doing most of the talking here, but there's one more thing I need to tell:

I've noticed that the GW team likes to use flags in many of their APIs. On the master branch, these flags are de-serialized into IEnumerable<TEnum>. Now I looked it up and I know that this is the standard way of handling JSON arrays. But really, flags should be treated as such. That's why I spent a couple of hours writing a reusable StringEnumFlagsConverter that can convert these arrays to and from bit flag enums. Feel free to use it.