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

core: move ConnectionResult out of DroneCore class #290

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

JonasVautherin
Copy link
Collaborator

Because it is used in many classes, ConnectionResult should not be nested into DroneCore, IMO. Moreover, having it separate makes the code a bit nicer (DroneCore::ConnectionResult becomes ConnectionResult).

Finally, I am mocking DroneCore for some unit tests and, in my situation, I need to have ConnectionResult separate from DroneCore.

@@ -0,0 +1,67 @@
#ifndef DRONECORE_CONNECTION_RESULT_H
#define DRONECORE_CONNECTION_RESULT_H
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually, I used #pragma once.

Copy link
Collaborator Author

@JonasVautherin JonasVautherin Feb 27, 2018

Choose a reason for hiding this comment

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

Oh, right. I did not know #pragma once actually, and I had seen some guidelines on isocpp.github.io, saying:

Some implementations offer vendor extensions like #pragma once as alternative to include guards. It is not standard and it is not portable.

Do you have an opinion on that? I really don't know, I just read it. I can change it for a pragma if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer it because it seems to work and it prevents errors when you copy and paste files without changing the define correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

@hamishwillee
Copy link
Collaborator

hamishwillee commented Feb 27, 2018

To me it feels a little wrong that the ConnectionResult is not part of DroneCore, since that is the one user facing place that manages connections. I'll take your work for it that this is a good idea though :-)

I assume the header is exported for docs generation purposes?

This will require quite a few docs changes.

@JonasVautherin
Copy link
Collaborator Author

Oh, I did not think about the docs changes =/. One reason is that I want to be able to mock DroneCore from the backend. So I want to have DroneCore in production, and MockDroneCore in testing. And both are independant classes that need ConnectionResult.

And also in terms of code, it's not like if DroneCore was hiding any of ConnectionResult given that it is used in the 40 files I had to edit :D.

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