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

feat: add C# client and example #8

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

frankracis
Copy link

Closes #6

I tried to keep it similar to how the C++ client was implemented.

The actual Client should be pretty easy to wire in to any app already using SimConnect.

@theomessin theomessin self-assigned this Apr 11, 2021
@theomessin theomessin changed the title Adding C# client feat: add C# client and example Apr 11, 2021
@theomessin
Copy link
Owner

theomessin commented Apr 11, 2021

Thanks for this, I'll try to review properly when I have some time available. A couple of comments/questions off the top of my head:

  1. Are these the C# way of doing DocBlocks?
/// <summary>
/// Handle a connection to the simulator
/// </summary>
/// <param name="sender">SimConnect Object</param>
/// <param name="data">Simulator info</param>
  1. Is your code using the C++ code in any way, or is this from scratch?
  2. It would be nice to have everything under a single Solution as opposed to having two separate ones. If we're going to have two separate solutions, I propose we clean up the folder structure.
  3. I see you based your code off of master as opposed to develop. Can you make sure you're using the same .editorconfig?
  4. It'd be nice if the code style roughly matched what we use for C++. We can probably use this for C#.

@frankracis
Copy link
Author

  1. Yep. That will create an XML file with the documentation that goes with the DLL
  2. From scratch. I obviously copied a lot from the C++ side, but it's not linking to it or doing any code generation. I made a small change from the original PR to be more consistent with "Request"
  3. Moved to main solution.
  4. Retargeted to develop
  5. This one's always touchy. That google style guide is a weird "Make C# look like C++" thing that I haven't run into before. Following the Microsoft Style Guide is much more common. Google referenced this, then changed a whole bunch of things. I updated the .editorconfig with the rules referenced in the Microsoft style guide, and updated my code to follow them. I'd rather follow these rules since it's much more in line with other C# project, but your house your rules, I'll change it over if you'd like.

@theomessin
Copy link
Owner

Hey @frankracis, thanks for your patience - I've been quite busy. I've had some time to think about this and I have two issues with this PR at the moment (nothing to do with your code 😄):

  1. The actual Jetbridge protocol will be evolving quite rapidly over the next few weeks. Since we are still in pre-release this will involve breaking changes, meaning that a lot of work would need to be done to keep this separate client up to date.
  2. I'm not sure whether having these clients in the same repo is the right thing to do - especially considering that I won't really be using the C# client personally so I can't really maintain it. I think that having a separate repo maintained by others would make more sense.

I'm open to discussing this further. Let me know what you think 😃

@frankracis
Copy link
Author

That makes sense - you don't want ownership of code when you're not going to be responsible for maintaining it.

I'll create my own repo for the C# client, and you can just keep a link to it here.

@davidbonnici1984
Copy link

That makes sense - you don't want ownership of code when you're not going to be responsible for maintaining it.

I'll create my own repo for the C# client, and you can just keep a link to it here.

I will be interested to use your client for opena3xx

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.

C# Software Development Kit
3 participants