search

omniaretail / nimator

Light-weight adhoc framework for creating monitoring apps with c-sharp based system-checks.
MIT License
10 stars 9 forks source link

Feature/creating new plugin #21

Closed RobertvBraam closed 4 years ago

RobertvBraam commented 7 years ago

I added a new project for CouchDb with two different checks:

My vision for this change is to make a nuget package extention for the Nimator package, so people with a couchDb can also install the Nimator.CouchDb package. There are still some improvement to be made, in terms of flexibility and usability, but for now I made a solid foundation to continue on.

jeroenheijmans commented 7 years ago

Even though I'm not actively involved in the development of Nimator anymore at the moment, I still read through this PR with interest. The idea of this kind of feature seems nice, but a few points to make:

  1. The PR and the code talks about "CouchDB", but the terminology in the code suggests it's actually about CouchBase, which is quite a different database system.
  2. I don't think it's fitting to have 1 PR that adds such a new package, and also changes the core packages. I think none of those changes to the other projects really belongs here. Specifically, I think the changes to what framework is targeted are not warrented (as an aside, the new project does target 4.5.1, whereas the change for e.g. the example app is to make it target 4.7).
  3. Not a biggie, but it seems wasteful to introduce FluentAssertions as an extra dependency, since NUnit has it's own style of fluent-like assertions already in use in the other test projects.
  4. As a matter of taste, I think the "//Arrange" etc comments aren't quite needed, as all developers familiar with unit tests will know about the AAA-format. (=> comments should be about "why", not about "what")
  5. Any particular reason you're testing async methods using synchronous tests? NUnit has some lovely async support too. (This is something that could also be changed after merging a PR.)
  6. I'm not sure if the CouchNimator class really belongs in the core framework code. At least I feel bootstrapping Nimator in an app is the responsibility of a consumer of our packages, not ours. It also seems like a strange mix to create layers FromJson and then add more using c#. Just leave creating the runner up to the framework, and use only json to bootstrap it.
  7. I very much like the idea of a separate set of NuGet packages with prefab checks. But I think we should follow what most people are doing: creating contrib projects as siblings to the official core framework. See e.g. the Ninject extensions projects or NHibernate contrib stuff.

Regardless of these points, it's very cool to get these contributions: thank you :D.

Also, I'd currently consider @WichardRiezebos to be the core maintainer, so regardless of my opinions I think he has final say in all this :)