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

Fix picnicapi search (again) #26

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

maartenpaul
Copy link

It seems that the Github API has been changed again. I'm not an expert Python developer, but tried to propose a working solution. Possibly it can be improved. I use it in combination with Home Assistant Picnic integration for which the search function was also broken (home-assistant/core#115351), now it seems to work again.

I have therefore tried to build upon the outstanding pull request from thijmen-j

#25

The Picnic API now returns a nested json structure which needs to be search through to find search results. I found this other project that makes use of the git and used their solution as inspiration.
https://github.com/simonmartyr/picnic-api/blob/main/search_page.go

thijmen-j and others added 5 commits April 30, 2024 21:58

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…point which returns a new format
…is version seems to yield all expected search results.
…ults. This version seems to yield all expected search results."

This reverts commit d09382e.
…" condition. This commit includes items that don't have a sole article id, and just sets the sole article id to None.

Also includes some minor reformatting and refactor (updated type hint Optional for functions that can return a None).
@maartenpaul maartenpaul changed the title Fix picnicapi branch Fix picnicapi search (again) Sep 23, 2024
@popeye1979
Copy link

Will this be approved/fixed soon? @corneyl

@corneyl
Copy link
Contributor

corneyl commented Oct 18, 2024

@popeye1979 I'm afraid I'm not a maintainer for this repo, so there's not much I can do.

@MikeBrink can you have a look or add someone as maintainer (or both)? Or would you advise to start a fork for further maintenance?

Copy link

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

The changes work for me 👍

test.py Outdated

Choose a reason for hiding this comment

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

I don't think this should be checked in. There is also a username and password in here.

There are integration tests that use environment variables for username+password.

@JanGiese
Copy link

works for me as well

@bobvanderlinden
Copy link

I emailed @MikeBrink about maintenance of this repo, but got no response. Maybe we should lean into forking or at least referring to this branch in Home Assistant?

Apart from that, would it be possible to place libraries as this one under the home assistant organization?

@corneyl
Copy link
Contributor

corneyl commented Jan 18, 2025

What about we start a GitHub organization, e.g. named home-assistant-pypi: the Home Assistant Python Package Index? When having multiple people being responsible for the organization, the chance that a repo is abandoned is almost zero.

@maartenpaul
Copy link
Author

Sounds like a cool idea, that would solve this kind of issue in the future!

At home-assistant/core there is now a parallel discussion about this:
home-assistant/core#115351 (comment)

If we can find somebody to take responsibility for the maintenance and fork the repository it would solve the problem for now. Since @MikeBrink is not responding for months, I guess we should just decide on this from the Home Assistant integration side.

I use it now as a custom integration and especially with voice control it is really nice to have this working!

@codesalatdev
Copy link

codesalatdev commented Feb 19, 2025

I forked the repository and merged this PR into it. I also fixed #27 (home-assistant/core#138735) and will keep updating the picnic-agent header whenever needed. Since I use Picnic on the regular, I could imagine myself maintaining this project.

The fork can be found at https://github.com/codesalatdev/python-picnic-api
PyPi: https://pypi.org/project/python-picnic-api2/

I am currently figuring out the setup and am already planning for some cleanup.

@glenndehaan
Copy link

@codesalatdev That sounds awesome. Are you also going to migrate the HA integration to your library?

@codesalatdev
Copy link

@codesalatdev That sounds awesome. Are you also going to migrate the HA integration to your library?

I am currently running my fork with a slightly modified version of the home assistant core integration (basically imports changed to python_picnic_api2) on my HA instance. However, I really want to wait for feedback from @corneyl since they are the codeowners for the core component. If they currently do not have time to modify the integration, I am open to create a PR, but they're probably more familiar with the changes required and I don't want to "circumvent" their decision-making :)

@maartenpaul
Copy link
Author

That would be great if you could maintain the package! I just tested it on my HA setup running picnic as custom component, and all works fine.

Maybe good to mention your repository and pypi package in this thread about the original issues with the API?
home-assistant/core#115351

@codesalatdev
Copy link

That would be great if you could maintain the package! I just tested it on my HA setup running picnic as custom component, and all works fine.

Nice!

Maybe good to mention your repository and pypi package in this thread about the original issues with the API?
home-assistant/core#115351

Will do :)

@corneyl
Copy link
Contributor

corneyl commented Feb 23, 2025

@codesalatdev nice that you picked this up!

I was also working on a fork for the past few days and didn't see any other recent forks, but missed yours for some reason. But I didn't get any further than fixing tests and making packages publish automatically to pypitest.

@codesalatdev
Copy link

@codesalatdev nice that you picked this up!

I was also working on a fork for the past few days and didn't see any other recent forks, but missed yours for some reason. But I didn't get any further than fixing tests and making packages publish automatically to pypitest.

If you want, I can add you as a collaborator to the PyPi project so that there's a second person with access to the project and you're obviously welcome to contribute :) Since you've approved the PR, I am assuming you do not want to use your own fork instead(?)

@thijmen-j
Copy link

@codesalatdev Since my PR was not being picked up I just copied the relevant code to my own project and I am keeping it updated there but would be happy to move back to a supported projects and also happy to contribute [I am keeping it updated anyways, might as well be for the common good ;)].

@codesalatdev
Copy link

@thijmen-j I might be totally misunderstanding your comment, but your code (the extract search function) is part of the forked library and currently used by Home Assistant >= 2025.3 :)

@thijmen-j
Copy link

@codesalatdev Haha, OK, that's great!

  1. Are you planning to maintain a repo (maybe it is https://github.com/codesalatdev/python-picnic-api) for the long term?
  2. If so, do you want me to help maintain it (bundle our efforts so to say)?

@codesalatdev
Copy link

1. Are you planning to maintain a repo (maybe it is https://github.com/codesalatdev/python-picnic-api) for the long term?

Absolutely. I've changed a large part of the development workflow, added trusted publishing for PyPi, added scheduled integration tests, fixed the unit tests and got myself access to an account based in NL to spot differences in the API. This setup is here to stay. Also: my washing machine orders its own detergent, so that should be an indication of my commitment ;)

2. If so, do you want me to help maintain it (bundle our efforts so to say)?

I plan to keep the library functional and add new features to it; but it's open source after all, so... sure? :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants