Skip to content

Add input_text component#9112

Merged
balloob merged 1 commit into
home-assistant:devfrom
BioSehnsucht:add-input-box
Sep 5, 2017
Merged

Add input_text component#9112
balloob merged 1 commit into
home-assistant:devfrom
BioSehnsucht:add-input-box

Conversation

@BioSehnsucht
Copy link
Copy Markdown
Contributor

@BioSehnsucht BioSehnsucht commented Aug 24, 2017

Description:

See also PR for polymer side of this feature : home-assistant/frontend#408

WIP, haven't written any documentation yet, but wanted to get feedback and relevant suggestions. This is pretty much finished up, I think, other than submitting a PR for documentation.

This implements an input box which can accept strings as well as integers/floats. I need this functionality to implement some features for my Elk M1 security/automation integration, and I figured better to make a generic component that others can use as well. Per PR discussions, only implementing string functionality. We will be renaming input_slider to input_numeric and giving it the ability to be slider or input box as a separate PR, which is what I'll end up using for my Elk integration generally rather than input_text.

Client side validation of min/max range on numbers not yet implemented, and it doesn't pass tox yet. Passes tox.

Specifically I have lint failures due to lines that are too long (I'm not sure the preferred way to shorten these, as some are long if .. and .. conditions and one is a really long string literal), as well as 'Anomalous backslash in string' for some regex's being passed into Polymer. I suppose I could move setting those default patterns to the Polymer side of things, but lint suggests turning them into python regex with r prefix, I suspect this is not actually a good idea since I'm not using them as regex in python itself? I may misunderstand the purpose of r prefix though. Passes tox.

There's also no tests implemented yet. Tests implemented.

I'm open to suggestions to change the name to something else, if anyone has a preference for it to be named something else. Was renamed to input_text per discussion in PR.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#3301

Just strings, no numbers (except that strings can contain numbers, of course, but they're going to be type str not float or int). Can specify min/max string length, as well as an optional pattern (since Polymer's input text box functionality supports this easily), all of which is client side validated.

Example entry for configuration.yaml (if applicable):

input_text:
  box1:
    name: Textbox String Test
    initial: String Test
  box2:
    name: Textbox Pattern Test 2
    initial: PatternTest
    pattern: '[a-zA-Z]*'
  box3:
    name: Textbox Pattern Test 3
    initial: 0932491
    pattern: '[0-9]*'
  box4:
    name: Textbox Hex Test 4
    initial: d34db33f
    pattern: '[a-fA-F0-9]*'
    min: 4
    max: 8

Checklist:

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

Comment thread homeassistant/components/input_box.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)

Comment thread homeassistant/components/input_box.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

Comment thread homeassistant/components/input_box.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (91 > 79 characters)

@cgarwood
Copy link
Copy Markdown
Member

Regarding the name, perhaps input_text or input_textbox?

Comment thread homeassistant/components/input_box.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why would we include those patterns in Python? We can just include them in the frontend only.

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 25, 2017

I agree, please rename to input_text.

I don't think that this component should support numbers/steps/minimum/maximum at all. People can use the input_slider for that. (which should have been called input_number but it's too late now.)

@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

@balloob
I'll change it to input_text since there's two votes for that.

Regarding patterns in frontend vs Python, I can make that work (would just have to pass the mode forward and set it in the frontend), but wanted to allow an end user (or someone extending the component) to specify a custom validation pattern if they had a use for it.

As for removing numbers & etc, I have a use for a numeric input which isn't a slider, because the range will be user-defined and can be from 0..65535 potentially, but more commonly is going to be well under 0..100 generally - however if using a larger range, using a slider to set to a precise number is cumbersome. I initially built this by copying input_slider, so steps are still in there, and I had considered adding an optional slider for numeric input (so you can use the slider or input a direct number) but I haven't taken the time to figure out how to put in two Polymer widgets together like that.

For simplicity, I'm not opposed to removing steps, but the entire reason I'm building this is to use it for primarily numeric input myself (though I have seen requests for generic text inputs / value storage so was enabling all use cases). Min/max for numeric input would be a min/max range just like it is on input_slider, while min/max for non-numeric input would be min/max limitations for text length in characters.

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 26, 2017

input_slider should be renamed to input_number and should allow an attribute that allows the frontend to switch between a slider and an input type number .

input_text should not do any number specific things. Probably also better to skip validation via patterns.

@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

@balloob Renaming input_slider may be quite the breaking change probably ? ... but enhancing it to allow either being slider or input box could be done without breakage

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 26, 2017

I think that we can justify the breaking change, to align all the input_* components.

@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

To summarize, proposed changes are:

  • input_slider : rename to input_number (breaking change)
  • input_number (formerly input_slider): add a configuration option to display input box instead of slider for UI (including implementing this functionality in the UI side)
  • input_box: rename to input_text
  • input_text (formerly input_box): remove number-related options
  • input_text (formerly input_box): remove regex pattern validation

I think we could still leave in the pattern validation as an optional thing on input_text, but not set in Python (or rather set to '' as default). Just allow a user to set it in the configuration if desired and then it's passed to the UI to use (otherwise the UI substitutes the empty pattern with [.*] or simply doesn't use validation). I don't have a particular use case off the top of my head, but it's easy to implement this way and I'm sure someone will appreciate it at some point. It would be processed entirely client-side, just passing the pattern setting if any from the configuration through to the UI.

@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

@balloob Do you want me to make all these changes in this PR including changes to input_slider -> input_number, etc, or do we want to handle that separately?

@dale3h
Copy link
Copy Markdown
Member

dale3h commented Aug 31, 2017

Please create a separate PR for change of input_slider -> input_number.

@dale3h dale3h changed the title Add input_box component Add input_text component Aug 31, 2017
@dale3h dale3h added this to the 0.54 milestone Aug 31, 2017
@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

Will do. I'll finish up input_text first then, then work on the input_number PR.

@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

BioSehnsucht commented Sep 3, 2017

Looks like Travis CI failed because Tox took too long to run ? I get various errors on unrelated code locally, but my code isn't currently throwing any tox error locally, I even have tests now...

edit: it re-ran itself and passed, looks like.

If nobody has any further suggested changes, I'll write up the documentation and submit a PR to the home-assistant.github.io repo for it next.

@dale3h
Copy link
Copy Markdown
Member

dale3h commented Sep 3, 2017

I restarted the build for you. Glad it passed! We are having a few timeouts lately, and I am not sure what the cause is.

@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

I just made some edits to the OP of PR, with updated yaml config examples and so on.

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Sep 3, 2017

Don't forget:

  • input_text (formerly input_box): remove number-related options
  • input_text (formerly input_box): remove regex pattern validation

@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

BioSehnsucht commented Sep 3, 2017

@pvizeli There's nothing specifically number related, not even the pattern validation functionality (if the user choses to use a regex pattern that only validates numbers that's on the user), which is just exposing underlying Polymer functionality. Even if you do so, you only get a str not int or float.

I can remove pattern validation if that's really what everyone wants, but why not leave it in for anyone who wants it? If you don't configure it, it's like it's not even there.

Min/max are for string length not numeric range limits.

There are no number-related configuration options.

@dale3h
Copy link
Copy Markdown
Member

dale3h commented Sep 3, 2017

Please leave the optional validation and optional min/max length config vars.

@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

Currently default string length min/max are 0/100, could increase default max to something much larger, or allow both to be zero and disable length checking at all in that case (and default both to zero), or just leave as-is.

@BioSehnsucht
Copy link
Copy Markdown
Contributor Author

Added PR for documentation.

Let me know whether the defaults and behavior for min/max length are fine as-is, or if I should change them, and if so I'll update the PRs accordingly. If defaults are fine currently, then I think input_text is complete (PRs for home-assistant itself, Polymer, and documentation repos).

@balloob balloob merged commit 552abf7 into home-assistant:dev Sep 5, 2017
@balloob
Copy link
Copy Markdown
Member

balloob commented Sep 5, 2017

Thanks for your patience with us and great work ! 👍 🐬

@balloob balloob mentioned this pull request Sep 7, 2017
@balloob balloob removed this from the 0.54 milestone Sep 7, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants