Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add state dependent icons to moon sensor #28743

Merged
merged 9 commits into from Nov 19, 2019
Merged

Conversation

@Mariusthvdb
Copy link
Contributor

Mariusthvdb commented Nov 13, 2019

Material design icons have icons for all sensor.states, let's use these natively in the component.
Based on the Season icons, tried to change accordingly.

Breaking Change:

Description:

Related issue (if applicable): fixes #

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Material design icons have icons for all sensor.states, let's use these natively in the component.
Based on the Season icons, tried to change accordingly.
@probot-home-assistant

This comment has been minimized.

Copy link

probot-home-assistant bot commented Nov 13, 2019

Hey there @fabaff, mind taking a look at this pull request as its been labeled with a integration (moon) you are listed as a codeowner for? Thanks!

Dev automation moved this from Needs review to Review in progress Nov 13, 2019
@MartinHjelmare MartinHjelmare changed the title have sensor.moon use state dependent icons Have sensor.moon use state dependent icons Nov 13, 2019
added state constants
fixed missing mdi:
Copy link
Member

fabaff left a comment

Run black locally before committing to avoid style issues.

homeassistant/components/moon/sensor.py Show resolved Hide resolved
Mariusthvdb added 3 commits Nov 13, 2019
change order of constants to alphabetical, changed order in Moon_icons to alphabetical, used self.state for icon lookup
 replace the strings for self.state with the constants
@Mariusthvdb

This comment has been minimized.

Copy link
Contributor Author

Mariusthvdb commented Nov 13, 2019

so other than running 'black' this is ok now?

@Mariusthvdb

This comment has been minimized.

Copy link
Contributor Author

Mariusthvdb commented Nov 14, 2019

need some assistance in how to check this in 'Black'. The link provided to the development_guidelines doesn't really help me tbh... Sorry for my noob-ness
Other than that, Afaik, I've fulfilled all change requests

@Mariusthvdb Mariusthvdb requested a review from fabaff Nov 14, 2019
@springstan

This comment has been minimized.

Copy link
Member

springstan commented Nov 15, 2019

@Mariusthvdb you need to format it with black e.g. black {source_file_or_directory}

@Mariusthvdb

This comment has been minimized.

Copy link
Contributor Author

Mariusthvdb commented Nov 15, 2019

thanks!
ok so I installed black locally and made a local copy of the pr'd file.

it errors out on an unaltered line, that has been there before my effort:

$ black /Users/theboss/Desktop/sensor.py 
error: cannot format /Users/theboss/Desktop/sensor.py: Cannot parse: 50:0:      def state(self):
Oh no! 💥 💔 💥
1 file failed to reformat.

now what. Really sorry, but I am afraid without reasonable instructions how to setup a local PR-test configuration this is all a bit too much for me...

@springstan

This comment has been minimized.

Copy link
Member

springstan commented Nov 15, 2019

@Mariusthvdb have you set up a dev environment or are you working without one?
Because inside of a dev environment you can simply run black --fast homeassistant to format your code and in addition pre-commit checks that your code is formatted with black whenever you commit.
See this dev blog post.

@Mariusthvdb

This comment has been minimized.

Copy link
Contributor Author

Mariusthvdb commented Nov 15, 2019

@Mariusthvdb have you set up a dev environment or are you working without one?
Because inside of a dev environment you can simply run black --fast homeassistant to format your code and in addition pre-commit checks that your code is formatted with black whenever you commit.
See this dev blog post.

thanks, I am working without one. Read the page before, and, using a Mac, installed home-brew, but after that, well, gave up really. Hence my effort to test the file in black locally.
It won't pass the test, on an unaltered line... Maybe it wasn't Black yet after all before Pr'd?
Hope someone else will takeover this PR, and manages to finish it.

Copy link
Member

springstan left a comment

This should fix your formatting errors :)

homeassistant/components/moon/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/moon/sensor.py Outdated Show resolved Hide resolved
Mariusthvdb added 2 commits Nov 15, 2019
@Mariusthvdb

This comment has been minimized.

Copy link
Contributor Author

Mariusthvdb commented Nov 15, 2019

not sure what the only failing check requires me to do now, I'll await further instructions.
file should be Black now, and all other requested changes are in place.

@Mariusthvdb

This comment has been minimized.

Copy link
Contributor Author

Mariusthvdb commented Nov 18, 2019

anyone can help with the codecov/project issue? final change to make things Black only added 1 comma, and suddenly this error appears...

@fabaff fabaff changed the title Have sensor.moon use state dependent icons Add state dependent icons to moon sensor Nov 18, 2019
Dev automation moved this from Review in progress to Reviewer approved Nov 18, 2019
@fabaff
fabaff approved these changes Nov 18, 2019
@Mariusthvdb

This comment has been minimized.

Copy link
Contributor Author

Mariusthvdb commented Nov 19, 2019

Anyone on why this is still unsuccessful ?

@frenck

This comment has been minimized.

Copy link
Member

frenck commented Nov 19, 2019

@Mariusthvdb The test coverage of this integration is not up to par, because you've added a couple of lines of code, which could also be tested.

https://github.com/home-assistant/home-assistant/blob/dev/tests/components/moon/test_sensor.py

Could you add a couple of tests to improve its coverage? That would be awesome!

@Mariusthvdb

This comment has been minimized.

Copy link
Contributor Author

Mariusthvdb commented Nov 19, 2019

o dear...
this only happened after I replaced the original code with the blackified code. And believe it or not, the only extra character was the final comma......

having said that, I will certainly have a look if I can add a test, I take it for the icon? Whole new territory and code-language for me, so not sure yet ;-)

ok, trying to KISS, and build on what's already there, could this be a way to go?:

    @patch("homeassistant.components.moon.sensor.dt_util.utcnow", return_value=DAY2)
    def test_moon_day2(self, mock_request):
        """Test the Moon sensor."""
        config = {"sensor": {"platform": "moon", "name": "moon_day2"}}

        assert setup_component(self.hass, "sensor", config)

        state = self.hass.states.get("sensor.moon_day2")
        assert state.state == "waning_gibbous"
        assert state.attributes.icon == "mdi:moon-waning-gibbous"

if so, Ill add it to the other test also.

@frenck

This comment has been minimized.

Copy link
Member

frenck commented Nov 19, 2019

Hmm.. looking at it, you've actually increased the coverage with 2.66%. So it's fine I guess.

The main issue (looking at codecov), is not the icons, but the other moon phases not being tested.

Considering that, this is all out of scope for this PR. So let's go ahead and merge this in.

If you are up for a little challenge and want to learn more about testing, I can definitely recommend this integration, since it is fairly simple and a good starter, with already some tests in place that simply need to be extended.

@frenck
frenck approved these changes Nov 19, 2019
@frenck frenck merged commit 5203c72 into home-assistant:dev Nov 19, 2019
10 of 11 checks passed
10 of 11 checks passed
codecov/patch 71.42% of diff hit (target 94.43%)
Details
CI Build #20191118.13 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/project 94.44% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Nov 19, 2019
@Mariusthvdb Mariusthvdb deleted the Mariusthvdb:patch-1 branch Nov 19, 2019
@Mariusthvdb

This comment has been minimized.

Copy link
Contributor Author

Mariusthvdb commented Nov 19, 2019

thanks Frenck, cool.
And will do. Hope to learn. Need to.....

@lock lock bot locked and limited conversation to collaborators Nov 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.