search

SignalK / specification

Signal K is a JSON-based format for storing and sharing marine data from different sources (e.g. nmea 0183, 2000, seatalk, etc)
Other
91 stars 69 forks source link

feature: add heatIndexTemperature to environment inside zones #447

Closed bkp7 closed 6 years ago

bkp7 commented 6 years ago

fixes #446.

Adds heatIndexTemperature to inside zones to be consistent with outside.

Also added a valid and invalid test.

bkp7 commented 6 years ago

@tkurki sorry, by flattening the additional changes you requested, I seem to have made your comments on this PR disappear.

A new PR #449 removes spaces from all existing filenames and also modifies the tests to ensure there's no recurrence.

I have modified the description wording in environment.json, and changed the filenames on this PR to remove the spaces.

tkurki commented 6 years ago

Github shows them ok to me, as outdated diffs that can be expanded.

I don't think the invalid example adds any value here: somebody might stumble on that and treat as an example, not an invalid one. Unless there is something I am missing?

BTW my pet peeve is commit messages (and PR titles) in the imperative mood per https://chris.beams.io/posts/git-commit/#imperative.

And while not a rule I have found Angular commit message conventions' helpful most of the time.

bkp7 commented 6 years ago

The invalid test is there for 2 purposes:

  1. to prove that the schema fails when an invalid property is present within a zone
  2. to ensure that heatIndex property explicitly fails as I felt this was actually a more intuitive name than heatIndexTemperature and it should be a test to ensure that the more intuitive name doesn't get put in in future, and also if someone does look through invalid tests they see that heatIndex is invalid by design.

I agree the second part of the test is of secondary importance to the first which ensures the schema is doing its job of failing invalid json. I can rename the test to make it clearer it's meant to fail or would you prefer I remove the secondary part of the test and change the property to an obviously spurious name?

Re the PR and commit naming, I'm certainly with you on keeping things consistent and standardised. I had looked at previous commit messages and it looked like everyone was doing their own thing.

I have added on new issue #450 to document some guidelines.

bkp7 commented 6 years ago

@timmathews, could I just clarify what the 'After v1' milestone is for please.

v1.0.0 was released on 1 Jan 2018, the current release is 1.0.3 released 8 Jan 2018.

If there's a freeze on new features whilst the bugs in v1 are ironed out that's great. It would be useful to be able to tag those bugs so we can get them cleared up.

Thanks

timmathews commented 6 years ago

@bkp7, "After v1" is essentially a feature freeze pending the official Signal K v1.0.0" announcement. Should be soon ....