Add minimum/maximum to counter by spacemanspiff2007 · Pull Request #20319 · home-assistant/core

spacemanspiff2007 · 2019-01-22T12:22:16Z

Docs: home-assistant/home-assistant.io#6802
OldPR: #17440

Description:

Example entry for `configuration.yaml` (if applicable):

counter:
  my_counter:
    initial: 0
    step : 1
    minimum: 0
    maximum: 10

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

Tests have been added to verify that the new code works.

dgomes · 2019-01-22T13:06:03Z

I know this to be a resubmittion of a PR that went wrong.

Still you need to reference the previous PR and add the proper description and reference to docs

spacemanspiff2007 · 2019-01-22T16:57:13Z

Updated description

dgomes · 2019-01-22T20:10:08Z

Need to fix your code (tests aren't passing)

rohankapoorcom · 2019-01-23T06:05:45Z

Looks like the test changes didn't make it into this one.

rohankapoorcom

Added some thoughts, mostly stuff where the incorrect changes were applied on top of dev.

rohankapoorcom · 2019-01-23T06:06:59Z

+})
+SERVICE_SCHEMA_SETUP = vol.Schema({
+    ATTR_ENTITY_ID: cv.entity_ids,
+    vol.Optional(CONF_MINIMUM): vol.Any(None, vol.Coerce(int)),


Is it worth setting the defaults to max_int and min_int respectively? That way there's no need for None checking in check_boundaries.

rohankapoorcom · 2019-01-23T06:07:14Z

 })

 CONFIG_SCHEMA = vol.Schema({
-    DOMAIN: cv.schema_with_slug_keys(


This should not have changed.

rohankapoorcom · 2019-01-23T06:07:46Z

-SERVICE_SCHEMA = vol.Schema({
-    vol.Optional(ATTR_ENTITY_ID): cv.comp_entity_ids,
+SERVICE_SCHEMA_SIMPLE = vol.Schema({
+    vol.Optional(ATTR_ENTITY_ID): cv.entity_ids,


This should not have changed.

I'm also confused at how this could be optional in the first place. The docs agree: https://www.home-assistant.io/components/counter/.

Assuming it should not be optional, SERVICE_SCHEMA_SETUP should extend SERVICE_SCHEMA_SIMPLE which I would recommend renaming to SERVICE_SCHEMA_BASE

rohankapoorcom · 2019-01-23T06:11:31Z

+            ret[CONF_MAXIMUM] = self._max
+        return ret
+
+    def __check_boundaries(self):


It feels cleaner if this returns a true/false rather than making the change directly.

And don't use double underscore in the beginning of the name. One underscore is enough.

rohankapoorcom · 2019-01-23T06:12:18Z



-class Counter(RestoreEntity):
+class Counter(Entity):


This should not have changed.

rohankapoorcom · 2019-01-23T06:13:15Z

+})
+SERVICE_SCHEMA_SETUP = vol.Schema({
+    ATTR_ENTITY_ID: cv.entity_ids,
+    vol.Optional(CONF_MINIMUM): vol.Any(None, vol.Coerce(int)),


It feels weird that these attributes are using CONF_ constants and not ATTR_ constants.

rohankapoorcom · 2019-02-10T17:44:01Z

@spacemanspiff2007 are you going to be able to make these changes? I would love to get these features merged in (would be super useful to me!)

spacemanspiff2007 · 2019-02-17T13:07:55Z

Hi,
I am actually abroad for the next four weeks. After that I'll take a look and try again.

robbiet480 · 2019-03-25T00:38:36Z

@spacemanspiff2007 Closing this until you have time to get back to it. Feel free to reopen it once you do have the time!

added minimum/maximum to counter

81e416a

spacemanspiff2007 requested a review from fabaff as a code owner January 22, 2019 12:22

homeassistant added integration: counter core cla-signed labels Jan 22, 2019

ghost added the in progress label Jan 22, 2019

spacemanspiff2007 mentioned this pull request Jan 22, 2019

Added minimum/maximum to counter #17440

Closed

3 tasks

frenck added the docs-missing label Jan 22, 2019

rohankapoorcom requested changes Jan 23, 2019

View reviewed changes

MartinHjelmare changed the title ~~added minimum/maximum to counter~~ Add minimum/maximum to counter Feb 1, 2019

robbiet480 closed this Mar 25, 2019

ghost removed the in progress label Mar 25, 2019

rohankapoorcom mentioned this pull request Apr 1, 2019

Add minimum/maximum to counter #22608

Merged

9 tasks



		class Counter(RestoreEntity):
		class Counter(Entity):

Uh oh!

Conversation

spacemanspiff2007 commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

dgomes commented Jan 22, 2019

Uh oh!

spacemanspiff2007 commented Jan 22, 2019

Uh oh!

dgomes commented Jan 22, 2019

Uh oh!

rohankapoorcom commented Jan 23, 2019

Uh oh!

rohankapoorcom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohankapoorcom commented Feb 10, 2019

Uh oh!

spacemanspiff2007 commented Feb 17, 2019

Uh oh!

robbiet480 commented Mar 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

spacemanspiff2007 commented Jan 22, 2019 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):