Skip to content

Conversation

@codesalatdev
Copy link
Contributor

Proposed change

This PR changes the python library of the picnic component (python-picnic-api) to my maintained fork python-picnic-api2. A fork of the underlying library has shown necessary, as the community was unable to contact the original maintainer (see MikeBrink/python-picnic-api#25 and MikeBrink/python-picnic-api#26 (comment)) which caused the component to be dysfunctional one way or another for the last ~10 months. With the component now unable to initialize at all and no one having access to the package on PyPi, I started to maintain a fork and am planning to continue doing that.

A rough changelog can be found here: https://github.com/codesalatdev/python-picnic-api/releases/tag/v1.2.1
PyPi: https://pypi.org/project/python-picnic-api2/

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)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Because of the dependency change and the fixes included in it, more than one issue is fixed implicitly.

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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format 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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @corneyl, mind taking a look at this pull request as it has been labeled with an integration (picnic) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of picnic can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign picnic Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@joostlek
Copy link
Member

So, did we do our due diligence and tried emailing them for example?

(oh lol, I opened the linked issue, forgot about that one)

@codesalatdev
Copy link
Contributor Author

codesalatdev commented Feb 23, 2025

So, did we do our due diligence and tried emailing them for example?

The second link in my "proposed changes" section takes you to a comment stating this, yes :(

(oh lol, I opened the linked issue, forgot about that one)

Fully understandable with the amount of time that passed :)

@codesalatdev
Copy link
Contributor Author

codesalatdev commented Feb 23, 2025

Seems like local tests successfully used python_picnic_api instead of python_picnic_api2 because of the initial devcontainer setup.

@codesalatdev
Copy link
Contributor Author

As far as I can see, tests are failing because of OneDrive

@dhoeben
Copy link

dhoeben commented Feb 24, 2025

Do you need help with updating the documentation or such?

@codesalatdev
Copy link
Contributor Author

Do you need help with updating the documentation or such?

There are no changes to how the integration functions and the docs do not mention the library used in the integration, so I am not aware of any changes that would be required.

I also can't really add tests for this change, since the only thing changed are the imports.

@joostlek
Copy link
Member

Okay so going forward, we want all dependencies to be created and published in the CI. As in, we require this for new integrations, but I think it would also be wise to start doing this now with a new dependency. This way we can be more confident that the code we use is the one stored in the repository, and not something someone made on their computer. So I am leaning towards the question if we are able to add it now.

@codesalatdev
Copy link
Contributor Author

Okay so going forward, we want all dependencies to be created and published in the CI. As in, we require this for new integrations, but I think it would also be wise to start doing this now with a new dependency. This way we can be more confident that the code we use is the one stored in the repository, and not something someone made on their computer. So I am leaning towards the question if we are able to add it now.

I am wondering about this from when I started using Home Assistant and can fully understand that source package availability on PyPi alone isn't cutting it.

What will this change entail? I am fully open to using this dependency change as a "test wagon" - if you want to have a more synchronised communication, feel free to contact me on Discord too (codesalat). I'd be interesting in talking about this!

@joostlek
Copy link
Member

In theory it would just be a CI action that you kick off and that it publishes that to PyPI. So it doesn't have to be much in theory

@codesalatdev
Copy link
Contributor Author

In theory it would just be a CI action that you kick off and that it publishes that to PyPI. So it doesn't have to be much in theory

And that CI should run under the core repository or the library's CI? I am planning the latter for python-picnic-api2 anyway, if that is everything there should be no issue.

@joostlek
Copy link
Member

The library's repository should do that yes. Be sure to look into trusted publishers. That makes it really easy to just define the repository and workflow in PyPI, and it will automatically figure everything out

@codesalatdev
Copy link
Contributor Author

The library's repository should do that yes. Be sure to look into trusted publishers. That makes it really easy to just define the repository and workflow in PyPI, and it will automatically figure everything out

Done:
https://github.com/codesalatdev/python-picnic-api/actions/runs/13522760181/job/37785680817
https://github.com/codesalatdev/python-picnic-api/actions/runs/13522760181/workflow

@joostlek
Copy link
Member

Can we use that version in here directly as well?

Bump to version that publishes to PyPi via trusted publishers
@codesalatdev
Copy link
Contributor Author

Can we use that version in here directly as well?

To my surprise, the job successfully ran with the same version number already configured in this PR; either because it built an identical package, or because it silently fails and does nothing.

No matter, I've just released a new patch version for only this purpose and bumped the manifest.

@joostlek
Copy link
Member

Check!

Also, if you want to help out as codeowner, feel free to add yourself to the list.

On that topic, @corneyl can you send me a message on discord?

@codesalatdev
Copy link
Contributor Author

Also, if you want to help out as codeowner, feel free to add yourself to the list.

Should be included in this PR :) b7fdb39

@joostlek
Copy link
Member

Oh, not all files in the PR have been regenerated, can you fix that?

Also be sure to send me a message on discord then

@codesalatdev
Copy link
Contributor Author

codesalatdev commented Feb 25, 2025

Oh, not all files in the PR have been regenerated, can you fix that?

Fixed. Pylint seems to have crashed (?) in CI though

@joostlek
Copy link
Member

Yea that was because something else. It got fixed now though, so I am fine with merging it with it failing

@joostlek joostlek merged commit 4e904bf into home-assistant:dev Feb 25, 2025
44 of 45 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 2025
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.

Picnic todo list broken

4 participants