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

Reorganize yeelight specs file #1166

Merged
merged 6 commits into from Nov 9, 2021
Merged

Conversation

Kirmas
Copy link
Contributor

@Kirmas Kirmas commented Oct 23, 2021

Today I understood that background light can have different params. (now its not true but who know). And theoretically new struct can do "background lamp" code simpler in the future.

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2021

Codecov Report

Merging #1166 (6a9bcb1) into master (9b099b5) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1166      +/-   ##
==========================================
+ Coverage   78.28%   78.32%   +0.03%     
==========================================
  Files          84       84              
  Lines        9520     9531      +11     
  Branches      776      778       +2     
==========================================
+ Hits         7453     7465      +12     
- Misses       1880     1881       +1     
+ Partials      187      185       -2     
Impacted Files Coverage Δ
miio/integrations/yeelight/__init__.py 87.78% <100.00%> (-0.17%) ⬇️
miio/integrations/yeelight/spec_helper.py 100.00% <100.00%> (ø)
...ations/yeelight/tests/test_yeelight_spec_helper.py 100.00% <100.00%> (ø)
miio/aqaracamera.py 65.21% <0.00%> (ø)
miio/airhumidifier_jsq.py 95.23% <0.00%> (+0.14%) ⬆️
miio/airhumidifier_mjjsq.py 93.47% <0.00%> (+0.21%) ⬆️
miio/miioprotocol.py 32.86% <0.00%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b099b5...6a9bcb1. Read the comment docs.

@Kirmas
Copy link
Contributor Author

Kirmas commented Oct 23, 2021

@rytilahti Could you please review this? I don't planning this PR and now it holding me.

@@ -16,14 +22,18 @@ class ColorTempRange(NamedTuple):


@attr.s(auto_attribs=True)
class YeelightModelInfo:
model: str
class YeelightLamplInfo:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
class YeelightLamplInfo:
class YeelightLampInfo:

value["background_light"],
value["supports_color"],
)
if "main" not in value:
Copy link
Owner

Choose a reason for hiding this comment

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

Considering there is no way not to have a main light, would it make sense to avoid the separate indent level and list main light features on the top-level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this so that the main and background light have the same structure. In my opinion, this is a more flexible option.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with that, even when I think the main interface should directly control the main light. I suppose I was just wondering if there could be a case where there is a main light & several sublights on a device.

miio/integrations/yeelight/specs.yaml Show resolved Hide resolved
@rytilahti
Copy link
Owner

rytilahti commented Oct 23, 2021

@rytilahti Could you please review this? I don't planning this PR and now it holding me.

I added a couple of comments, but you know that you can always rebase another branch (and PR) on top of an existing one to avoid hold ups? :-)

@Kirmas
Copy link
Contributor Author

Kirmas commented Oct 23, 2021

@rytilahti Could you please review this? I don't planning this PR and now it holding me.

I added a couple of comments, but you know that you can always base another branch (and PR) on top of an existing one to avoid hold ups? :-)

Honestly don't know. Thank you for something new.

@rytilahti
Copy link
Owner

Honestly don't know. Thank you for something new.

You are welcome, if you want to know more, take a look at https://git-scm.com/book/en/v2/Git-Branching-Rebasing - it gets a bit complicated when working with several branches, but with two branches (+ master) you can do something like this:

git checkout -b some_feature
<work on some_feature>
git commit -m "implemented some_feature"
...
git checkout -b <second_feature> # using -b creates a new branch on top of the current one, in this case `some_feature`
<work on second_feature, commit things>
...
git checkout <some_feature> # move back to the initial branch
<work on some other changes>
git commit -m "implemented more changes to some_feature"
...
git checkout <second_feature>
git rebase <some_feature> # moves the commits of <second_feature> to be based on the <some_feature> branch

One thing to notice: as rebasing will rewrite the commit history (by adding those commits in-between), using it makes it necessary to do a force push when pushing changes to a remote which loses the existing history of that branch (and e.g., github comments).

@rytilahti rytilahti changed the title Reorganizing information in a specs file Reorganize yeelight specs file Nov 9, 2021
@rytilahti rytilahti merged commit 452d436 into rytilahti:master Nov 9, 2021
@Kirmas Kirmas deleted the yeelight_spec branch November 9, 2021 12:39
@rytilahti rytilahti mentioned this pull request Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants