Skip to content

Fix tankerkoenig with more than 10 stations#33098

Merged
balloob merged 2 commits into
home-assistant:devfrom
guillempages:tankerkoenig
Mar 21, 2020
Merged

Fix tankerkoenig with more than 10 stations#33098
balloob merged 2 commits into
home-assistant:devfrom
guillempages:tankerkoenig

Conversation

@guillempages
Copy link
Copy Markdown
Contributor

There seemed to be a problem, if more than 10 fuel stations were tracked.
Added a warning in this case, and split the calls to the API in chunks of
10, so that the data can be fetched anyway.

Proposed change

Fix issue #33061

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
tankerkoenig:
  api_key: my-key
  fuel_types: 
    - diesel
  radius: 5
  scan_interval: "0:10:00"

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

Comment thread homeassistant/components/tankerkoenig/__init__.py Outdated
There seemed to be a problem, if more than 10 fuel stations were tracked.
Added a warning in this case, and split the calls to the API in chunks of
10, so that the data can be fetched anyway.
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Good! Just a clean up.

Comment thread homeassistant/components/tankerkoenig/__init__.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

Should we tag this for a patch release?

@guillempages
Copy link
Copy Markdown
Contributor Author

I'd say yes. The integration was just added on the 0.107 release, so fixing its newborn bugs soon is good IMHO ;-)

There might be a merge conflict later on with the UUID validator PR that was early accepted today (#32805); might be worth putting that in as well. But AFAIK, that's not my call ;-)

Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>
@MartinHjelmare
Copy link
Copy Markdown
Member

Ok, since the other PR has already been merged to dev, I'm not sure we can apply this cleanly to master.

If you still want to try, please verify how a cherrypick of this PR would work on master branch.

@guillempages
Copy link
Copy Markdown
Contributor Author

The code would work OK (I actually implemented and tested it without the other one); there is just a minor conflict in the imports; since both PRs are adding a new import. Should be trivial to fix (I actually rebased and had the commit myself), but must be done manually. I don't know what's the policy here, or who should then do the merge.

@MartinHjelmare
Copy link
Copy Markdown
Member

I don't know if we accept merge conflicts during cherry-picking. We want to do it automatically with this script:
https://github.com/home-assistant/hass-release/blob/c1b6c166660da592174ee882d4ded458a0d03bcb/hassrelease/commands.py#L55

@balloob ?

@guillempages
Copy link
Copy Markdown
Contributor Author

If you want to do it fully automated, the easiest would be to cherry-pick both #32805 and this. The other PR is just improving the config validation, so it shouldn't add any new bugs. But let's @balloob decide.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 21, 2020

Will pick both.

@balloob balloob added this to the 0.107.5 milestone Mar 21, 2020
@balloob balloob merged commit 79f6d55 into home-assistant:dev Mar 21, 2020
balloob pushed a commit that referenced this pull request Mar 21, 2020
* Fix tankerkoenig with more than 10 stations

There seemed to be a problem, if more than 10 fuel stations were tracked.
Added a warning in this case, and split the calls to the API in chunks of
10, so that the data can be fetched anyway.

* Update homeassistant/components/tankerkoenig/__init__.py

Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@balloob balloob mentioned this pull request Mar 21, 2020
@guillempages guillempages deleted the tankerkoenig branch March 21, 2020 22:20
balloob pushed a commit that referenced this pull request Mar 21, 2020
* Fix tankerkoenig with more than 10 stations

There seemed to be a problem, if more than 10 fuel stations were tracked.
Added a warning in this case, and split the calls to the API in chunks of
10, so that the data can be fetched anyway.

* Update homeassistant/components/tankerkoenig/__init__.py

Co-Authored-By: Martin Hjelmare <marhje52@gmail.com>

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 21, 2020

Aah so in the end with the conflict because the other validation was bad and required another PR to fix it in the same hotfix.

@guillempages
Copy link
Copy Markdown
Contributor Author

yes, sorry again. #32805 should be completely removed (also from dev). Should I do a PR reverting it, or will you do some git magic behind the scenes?

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 21, 2020

PR to revert it.

@guillempages
Copy link
Copy Markdown
Contributor Author

Done: #33123

@lock lock Bot locked and limited conversation to collaborators Mar 27, 2020
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.

tankerkoenig not updating sensors

4 participants