[scraper] Pass JSON serialized path settings to python scrapers #17063

Montellese · 2019-12-21T11:02:47Z

Description

This is an alternative approach to #16964 and uses the newly introduce ISettingsValueSerializerInterface from #17062. It converts the scraper path settings to flat JSON like e.g.

{
  "RatingS": 1,
  "absolutenumber": false,
  "dvdorder": false,
  "language": "en",
  "token": ""
}

and passes them to the python based scrapers.

This will need to be backported to Leia together with the following commit from #17062: 7fbb037

Motivation and Context

See #16964. I prefer to use a clean approach / solution.

How Has This Been Tested?

Manually.

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

ronie · 2019-12-21T20:50:12Z

Thx, tested & works great.

rmrector · 2019-12-21T21:19:06Z

Thanks. Can you return an empty object/dictionary, {}, in the failure conditions rather than an empty string? This way it will always deserialize to a dictionary in Python.

Montellese · 2019-12-21T23:45:02Z

I added handling for empty serializations in CScraper::GetPathSettingsAsJSON().
I accidentally deleted the branch of this PR so it was automatically closed. I'll try to re-open it.

rmrector

I runtime tested it and it works well. The new interface looks great. Thanks.

Rechi

Please follow the current code guidelines.

xbmc/settings/lib/ISettingsValueSerializer.h

xbmc/settings/SettingsValueXmlSerializer.cpp

xbmc/settings/SettingsValueFlatJsonSerializer.cpp

Rechi · 2019-12-23T08:31:54Z

xbmc/addons/Scraper.cpp

@@ -800,10 +819,12 @@ static bool PythonDetails(const std::string &ID,
                          const std::string &key,
                          const std::string &url,
                          const std::string &action,
+                          const std::string &pathSettings,


Left-align * and & to the base type they modify.

https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md#52-pointer-and-reference-types

Is it really the intention to use a different formatting style on the newly added parameter than for all the other parameters?

You could also adjust the style of the already existing parameters to match the current code guidelines.

Which has absolutely nothing to do with this PR.

Montellese · 2019-12-23T23:16:03Z

Addressed almost all style issues.

garbear

JSON implementation looks good

Montellese · 2019-12-25T09:22:15Z

@ronie @romanvm @rmrector are you ok with the current implementation?

romanvm · 2019-12-25T19:16:41Z

@Montellese Sorry I had no time to check or test your code but from a cursory glance it looks better than my amateur approach. I guess I'll close my PR.

ronie

tested again with the latest cosmetic changes and it still work ok.
cheers!

romanvm

To be honest, I have limited understanding of Kodi settings internals, but the scraper part looks good.

…ializer

Montellese · 2019-12-31T23:54:14Z

Rebased after merging #17062 and waiting for Jenkins results before merging.

rmrector · 2020-01-01T16:26:14Z

jenkins build this please

Montellese · 2020-01-01T21:47:09Z

See #17112 for the backport for Leia.

…erializer [scraper] Pass JSON serialized path settings to python scrapers

Montellese added Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality Backport: Needed Component: Settings Component: Add-ons v19 Matrix labels Dec 21, 2019

Montellese added this to the Matrix 19.0-alpha 1 milestone Dec 21, 2019

Montellese added this to In progress in General (roadmap) via automation Dec 21, 2019

Montellese requested review from romanvm, ronie and rmrector December 21, 2019 11:03

Montellese mentioned this pull request Dec 21, 2019

[scrapers] Pass path settings to Python scrapers as JSON #16964

Closed

13 tasks

Montellese closed this Dec 21, 2019

Montellese deleted the feature/settings_json_serializer branch December 21, 2019 23:38

General (roadmap) automation moved this from In progress to Done Dec 21, 2019

Montellese moved this from Done to In progress in General (roadmap) Dec 21, 2019

Montellese restored the feature/settings_json_serializer branch December 21, 2019 23:44

Montellese reopened this Dec 21, 2019

Montellese force-pushed the feature/settings_json_serializer branch 3 times, most recently from 7b5b6b8 to 2812ce4 Compare December 21, 2019 23:53

rmrector reviewed Dec 23, 2019

View reviewed changes

Rechi reviewed Dec 23, 2019

View reviewed changes

Montellese force-pushed the feature/settings_json_serializer branch from 2812ce4 to 13d7a51 Compare December 23, 2019 23:13

garbear approved these changes Dec 24, 2019

View reviewed changes

General (roadmap) automation moved this from In progress to Reviewer approved Dec 24, 2019

Montellese force-pushed the feature/settings_json_serializer branch from 13d7a51 to d9d0bea Compare December 25, 2019 09:21

rmrector approved these changes Dec 25, 2019

View reviewed changes

ronie approved these changes Dec 25, 2019

View reviewed changes

romanvm approved these changes Dec 31, 2019

View reviewed changes

Montellese added 2 commits January 1, 2020 00:52

[settings] add JSON serialization implementation of ISettingsValueSer…

5159e7e

…ializer

pass path settings as JSON to python scrapers (thanks @romanvm)

47d015e

Montellese force-pushed the feature/settings_json_serializer branch from d9d0bea to 47d015e Compare December 31, 2019 23:53

Montellese merged commit bcd4533 into xbmc:master Jan 1, 2020

General (roadmap) automation moved this from Reviewer approved to Done Jan 1, 2020

Montellese deleted the feature/settings_json_serializer branch January 1, 2020 18:00

rmrector mentioned this pull request Jan 1, 2020

Path specific settings xbmc/metadata.themoviedb.org.python#18

Merged

Montellese added the Wiki: Needed label Jan 1, 2020

Montellese mentioned this pull request Jan 1, 2020

[backport] [scraper] Pass JSON serialized path settings to python scrapers #17112

Merged

13 tasks

Montellese added Backport: Done and removed Backport: Needed labels Jan 1, 2020

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jan 21, 2020

Merge pull request xbmc#17063 from Montellese/feature/settings_json_s…

3802f36

…erializer [scraper] Pass JSON serialized path settings to python scrapers

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[scraper] Pass JSON serialized path settings to python scrapers #17063

[scraper] Pass JSON serialized path settings to python scrapers #17063

Montellese commented Dec 21, 2019

ronie commented Dec 21, 2019

rmrector commented Dec 21, 2019

Montellese commented Dec 21, 2019

rmrector left a comment

Rechi left a comment

Rechi Dec 23, 2019

Montellese Dec 23, 2019

Rechi Dec 24, 2019

Montellese Dec 25, 2019

Montellese commented Dec 23, 2019

garbear left a comment

Montellese commented Dec 25, 2019

romanvm commented Dec 25, 2019 •

edited

Loading

ronie left a comment

romanvm left a comment

Montellese commented Dec 31, 2019

rmrector commented Jan 1, 2020

Montellese commented Jan 1, 2020

[scraper] Pass JSON serialized path settings to python scrapers #17063

[scraper] Pass JSON serialized path settings to python scrapers #17063

Conversation

Montellese commented Dec 21, 2019

Description

Motivation and Context

How Has This Been Tested?

Types of change

Checklist:

ronie commented Dec 21, 2019

rmrector commented Dec 21, 2019

Montellese commented Dec 21, 2019

rmrector left a comment

Choose a reason for hiding this comment

Rechi left a comment

Choose a reason for hiding this comment

Rechi Dec 23, 2019

Choose a reason for hiding this comment

Montellese Dec 23, 2019

Choose a reason for hiding this comment

Rechi Dec 24, 2019

Choose a reason for hiding this comment

Montellese Dec 25, 2019

Choose a reason for hiding this comment

Montellese commented Dec 23, 2019

garbear left a comment

Choose a reason for hiding this comment

Montellese commented Dec 25, 2019

romanvm commented Dec 25, 2019 • edited Loading

ronie left a comment

Choose a reason for hiding this comment

romanvm left a comment

Choose a reason for hiding this comment

Montellese commented Dec 31, 2019

rmrector commented Jan 1, 2020

Montellese commented Jan 1, 2020

romanvm commented Dec 25, 2019 •

edited

Loading