Extract external libraries

[baphled]

This PR addresses the fact that there are a few classes tucked away inside ConfigManager.h.

It moves BaseParameter, and it's associated classes, into their own header file. As well as moving the debugging definitions into its own file.

We also introduce a few unit tests to help drive some of the changes.

At present these tests require a ESP8266 and ESP32 board to run the tests against. This is due to the requirement of the Arduino library which ArduinoJson relies upon.

This could be changed to running the tests natively, once ArduinoJson is replaced with a JSON library that is not reliant on Arduino.h header file, allowing us to integrate unit testing into our TravisCI build.

We could still use the current setup, so that we can test ConfigManager against real boards, so this seems like a sane step to make to get the library under test.

[baphled]

@nrwiersma and @cgmckeever

I found that there is an inconsistency between clearing data for Strings and other data types. I've added a test for each, so we can see the difference.

Although I'll leave it with you guys to determine which one is actually correct :)

I think it'd be nice to be able to have some of the core functionality under test, which would help spot small details like this.

One thing that make this a bit interesting is that we have to compile our tests against a development board, as we require ArduinoJson . Which in turn requires the Arduino libraries. We could use a native JSON library to get around this, which would mean our parameter classes could be tested in isolation.

We could then use the current unit test setup to run tests specifically for ConfigManager allowing us to make sure that the core ConfigManager functionality works as expected, per board. Whilst being able to test its dependancies quickly, and separately.

What do you guys think?

[nrwiersma]

Hi,

Thanks for the PR. I am not keen on splitting out separate header files. I dont see it bringing anything to the party.

The tests however are very interesting. I think they will make an great start to a full test suite. I would also like to explore the possibility of switching the json lib.

Would you be open to removing the ConfigManager changes, and make this PR about the test suite?

[baphled]

Heya, soz, I'm just getting back into things.

Sure, I could revert the header extraction, although I do think there's utility in splitting up the files as it makes it easier to test them individually. As well as it's easier to work on the main piece of the library.

As it's pretty standard to have headers in their individual files, so I adopted it here also. To be fair, if I didn't split the definitions out, I'm not sure that little bug would have jumped out at me 😆

I also think that our test files will start getting a little crazy if we have just one file for all class definitions, stepping away more from the standardised way of writing tests, 1 test file per class, and coming up with a alternative to get around our want to keep all definitions and implementation in one file 🤔 🤷‍♂

I think it's appropriate to start to move the definitions for a number of reasons.

• C++ Standardisation
• Easier to write tests, as you only need to think about the class at hand
• It extracts classes that aren't directly needed by users out of the user space.
• Our tests are cleaner as they only focus and require the class definition we're testing.
• Class definitions are easier to find and read, as we don't have to skim through ConfigManager.h
• Can tests each lib without necessarily needed to rely on the Arduino ecosystem (meaning faster tests)
  • This will be more relevant once we replace ArduinoJson with a native C++ alternative.

I'll let you have a mull over it, I'll be away for a few more days but I should be around again, after the 16th.

[nrwiersma]

All fair points.

My problem starts when you start shifting things that should be in src into lib which i would argue is technically wrong. I am also not sure that DebugPrint folder is really necessary. Past that, you have made some good points.

[baphled]

I'm certainly with you in regards to DebugPrint. I was thinking about moving it to the include directory but I’m not sure about that either. It is a helper macro and it’s only really needs to be accessed within this library 🤔🤷‍♂️

I get where you’re coming from, although, if we think of it as being needed only by the ConfigManager class and not directly by users. It could be argued it’s a private library.

http://docs.platformio.org/en/latest/projectconf/section_platformio.html#lib-dir

I could fully be missing something though, my C/C++ isn’t as strong as my knowledge of other languages :)

[nrwiersma]

I would argue that the lib folder is for external libs. Even if private (only used by this project) they are still seen as something that can be made into their own library in its own right. Everything in ConfigManager today is direct source, and all belongs in src. Every line is part of ConfigManager.

So to give any credence to splitting the into multiple files, they must all be in src or some structure under src. Given the relative simplicity of ConfigManager at this point, no sub-structure is really needed IMO.

Granted, all of this is likely opinionated.

[baphled]

@nrwiersma valid point, and apologies for being slow on this. Hopefully I'll make some changes in the coming days :)

[cgmckeever]

@baphled so .. forgetting about this PR, I started to go down some similar paths with the files .. and then did unroll them as it seemed to over complicate it a bunch. I wish I was able to remember this PR when I started :) .. still working through this?

[baphled]

@nrwiersma ah, that’s no problem. I was planning on finishing this off then got hit by a load of dates to platformio. I’m curious as to what these complications looked like, but I’m cool for you to drop this if you think it’s not going to improve the Lib :)

…

Sent from my iPhone

On 2 Aug 2020, at 22:38, cgmckeever ***@***.***> wrote:  @baphled so .. forgetting about this PR, I started to go down some similar paths with the files .. and then did unroll them as it seemed to over complicate it a bunch. I wish I was able to remember this PR when I started :) .. still working through this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[cgmckeever]

@baphled I think some of this discussion has made its way over to here

[baphled]

@cgmcrkeeper awesome, much appreciated. That’s another thing I’ve wanted to address 😁

…

Sent from my iPhone

On 6 Aug 2020, at 16:52, cgmckeever ***@***.***> wrote:  @baphled I think some of this discussion has made its way over to here — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.