Skip to content

Add missing time.h include, use relative include path on time.h includes #6730

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

Merged
merged 3 commits into from
Nov 8, 2019
Merged

Conversation

Niek
Copy link
Contributor

@Niek Niek commented Nov 8, 2019

Added missing time.h include. The path is relative (instead of simply using <time.h>) so it doesn't conflict with Time.h of the Arduino Time library. See #6714 (comment) for details.

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 8, 2019

You want to change all time.h includes?

Yes !

@Niek
Copy link
Contributor Author

Niek commented Nov 8, 2019

You want to change all time.h includes?

Yes !

Done, except for the examples - since those don't use external libraries anyways I don't think it makes sense to change those (CMIIW).

@Niek Niek changed the title Add missing time.h include Add missing time.h include, use relative include path on time.h includes Nov 8, 2019
@d-a-v
Copy link
Collaborator

d-a-v commented Nov 8, 2019

CMIIW

If you have this library, windows or macOS, and if you can compile&run these examples, then it's fine.
Is the initial include in FS.h necessary when the others are modified ?

@Niek
Copy link
Contributor Author

Niek commented Nov 8, 2019

If you have this library, windows or macOS, and if you can compile&run these examples, then it's fine.

That's the case, unless you specifically link it against the Time library (which for an example would be weird).

Is the initial include in FS.h necessary when the others are modified ?

In my opinion FS.h should have a time.h include, because it calls time(). You can't really rely on the parent code to include the header file.

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

please add a comment on each changed line stating compat with Time lib, see issue #6714 or along those lines.

@Niek Niek requested a review from devyte November 8, 2019 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants