dylandoamaral / trakt-integration

A Trakt integration for Home Assistant compatible with upcoming media card
MIT License
30 stars 12 forks source link

Attempted to add Anticipated Shows and Anticipated Movies sensors #110

Closed mkanet closed 2 weeks ago

mkanet commented 1 month ago

I really appreciate your help to add Anticipated Shows and Anticipated Movies sensors. Once I'm more familiar with the structure of your code, I will be able to submit PRs more easily.

PS: I will remove the comments I added soon. I had it there temporarily so I can easily spot the code I added.

mkanet commented 1 month ago

Thank you. I tried my best to make the changes you requested. See my second commit: https://github.com/dylandoamaral/trakt-integration/pull/110/commits/c1dac6404c824d2db8b1599ee209d8f263874dd8. Please forgive me if I didn't do something correctly. The good news is that I am not getting any error messages in home assistant log when adding this integration via config_flow.

The bad news is that the new sensors are still not being created on my Home Assistant instance after adding this modified trakt integration via config_flow. Only the existing sensors that were there before are being created.

Below, is the configuration I am specifying in configuration.yaml.

trakt_tv:
  language: en
  timezone: America/Los_Angeles
  sensors:
    upcoming:
      new_show:
        days_to_fetch: 60
        max_medias: 10
      movie:
        days_to_fetch: 60
        max_medias: 10
      dvd:
        days_to_fetch: 60
        max_medias: 10
    recommendation:
      show:
        max_medias: 10
      movie:
        max_medias: 10
    anticipated:
      anticipated_movie:
        max_medias: 10
      anticipated_show:
        max_medias: 10

Can you please tell me what I need to change or add to the existing source files so the integration will actually create the 2 new sensors: anticipated_movies and anticipated_shows? Thank you for all your help and patience!

dylandoamaral commented 1 month ago

Thank you. I tried my best to make the changes you requested. See my second commit: c1dac64. Please forgive me if I didn't do something correctly. The good news is that I am not getting any error messages in home assistant log when adding this integration via config_flow.

The bad news is that the new sensors are still not being created on my Home Assistant instance after adding this modified trakt integration via config_flow. Only the existing sensors that were there before are being created.

Below, is the configuration I am specifying in configuration.yaml.

trakt_tv:
  language: en
  timezone: America/Los_Angeles
  sensors:
    upcoming:
      new_show:
        days_to_fetch: 60
        max_medias: 10
      movie:
        days_to_fetch: 60
        max_medias: 10
      dvd:
        days_to_fetch: 60
        max_medias: 10
    recommendation:
      show:
        max_medias: 10
      movie:
        max_medias: 10
    anticipated:
      anticipated_movie:
        max_medias: 10
      anticipated_show:
        max_medias: 10

Can you please tell me what I need to change or add to the existing source files so the integration will actually create the 2 new sensors: anticipated_movies and anticipated_shows? Thank you for all your help and patience!

Thank YOU, the effort is appreciated a lot, I will look at it tomorrow for sure ;).

downey-lv commented 1 month ago

Good job on the improvements! Added some more comments to help you out

The bad news is that the new sensors are still not being created on my Home Assistant instance after adding this modified trakt integration via config_flow. Only the existing sensors that were there before are being created.

The missing definition of ANTICIPATED_KINDS would definitely break the code, could be the reason why you're not seeing any sensors. Once you fix that you can go into http://localhost:8123/config/logs?filter=trakt_tv (change base URL to your instance) and see if anything else is throwing you any errors

mkanet commented 1 month ago

@downey-lv thank you so much for being so patient with me. I really appreciate that you guys are so friendly. I tried my best to make the changes you suggested: https://github.com/dylandoamaral/trakt-integration/pull/110/commits/c95d4743e635413dd5fb549790328b7ccaf2225d

I also verified that there are no errors in home assistant log. image

Unfortunately, the new sensors are still not being created after adding this modified integration via config_flow with no errors at all. Only the sensors I had before are getting created: image

There is something fundamental that I am missing. Can you please tell me what I should try next?

downey-lv commented 1 month ago

I pulled the commit, made the changes and the sensors work properly

Just FYI, I did not test exclude_collected

image

mkanet commented 1 month ago

thank you. Yes, it worked for me too. thank you very much! Honestly, Ive never used the exclude_collected feature before with this integration. Ill see what kind of effect it will have on the sensor data with it enabled.

mkanet commented 1 month ago

@downey-lv can you please tell me the minimum code i need to change so that the configuration options are consistent with other configuration settings? I just realized that

    anticipated:
      anticipated_movie:
        max_medias: 10
      anticipated_show:
        max_medias: 10

is not consistent with other options. it should be

    anticipated:
      movie:
        max_medias: 10
      show:
        max_medias: 10
downey-lv commented 4 weeks ago

i need to change so that the configuration options are consistent with other configuration settings?

You can't just have movie and show there, because that would lead to duplicate identifiers. You could change the config schema to have movie/show there and then remap them to anticipated_x in sensor.py instead of having anticipated_x in config. @dylandoamaral what do you prefer?

mkanet commented 3 weeks ago

@downey-lv Thank you so much for all your help. As soon as I have some free time I will make the changes and push another commit. I can't wait. I really like these new sensors!

mkanet commented 3 weeks ago

Hi @downey-lv I tried to do this on my own; but, I'm having a lot of trouble to modify the latest commit I provided in order to support a configuration such below. I didnt unfortunately understand what you suggested I need to do in order to accomplish this.

trakt_tv:
  language: en
  timezone: America/Los_Angeles
  sensors:
    anticipated:
      movie:
        max_medias: 10
      show:
        max_medias: 10

I think it would be better than keeping it like:

trakt_tv:
  language: en
  timezone: America/Los_Angeles
  sensors:
    anticipated:
      anticipated_movie:
        max_medias: 10
      anticipated_show:
        max_medias: 10

I could really use your help to achieve this; or whatever would be consistent with existing configuration options.

mkanet commented 2 weeks ago

Hi @downey-lv and @dylandoamaral I have made all the changes on my own: 8c6d769e2e64ef085cc06cf9afc77b096c85afd0. They work correctly as expected for me when using:

trakt_tv:
  language: en
  timezone: America/Los_Angeles
  sensors:
    anticipated:
      movie:
        max_medias: 10
      show:
        max_medias: 10

It creates the sensors: sensor.trakt_anticipated_shows and sensor.trakt_anticipated_movies. I think I also removed all the temporary comments I had added.

dylandoamaral commented 2 weeks ago

Hi @downey-lv and @dylandoamaral I have made all the changes on my own: 8c6d769. They work correctly as expected for me when using:

trakt_tv:
  language: en
  timezone: America/Los_Angeles
  sensors:
    anticipated:
      movie:
        max_medias: 10
      show:
        max_medias: 10

It creates the sensors: sensor.trakt_anticipated_shows and sensor.trakt_anticipated_movies. I think I also removed all the temporary comments I had added.

Hello @mkanet you also have to merge the main branch into your branch and resolve conflicts. I will be in holliday next week but then I will be able to review it !

mkanet commented 2 weeks ago

I will submit a new PR for this and close this one.