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

add new config values #4635

Closed
wants to merge 1 commit into from
Closed

Conversation

OmgImAlexis
Copy link
Collaborator

I can't find anything using it but animeSplitHome and animeSplitHomeInTabs are now apart of the anime field.

@OmgImAlexis OmgImAlexis added Enhancement Concluded Needs review Needs testing Requires testing to make sure it's working as intended labels Jul 8, 2018
@OmgImAlexis OmgImAlexis added this to the 0.2.7 milestone Jul 8, 2018
@OmgImAlexis OmgImAlexis requested a review from sharkykh July 8, 2018 11:09
section_data['nmj']['mount'] = app.NMJ_MOUNT

section_data['anime'] = NonEmptyDict()
section_data['anime']['enabled'] = bool(app.ANIMESUPPORT)
Copy link
Contributor

Choose a reason for hiding this comment

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

app.ANIMESUPPORT has no real meaning (it's never used). It's better to just not add it.

section_data['anime'] = NonEmptyDict()
section_data['anime']['enabled'] = bool(app.ANIMESUPPORT)
section_data['anime']['splitHome'] = bool(app.ANIME_SPLIT_HOME)
section_data['anime']['splitHomeInTabs'] = bool(app.ANIME_SPLIT_HOME_IN_TABS)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't put these under anime.
I would put these under layout.
Possibly, layout.anime.*
BTW, don't get fooled by the home in the name. I noticed it also affected the show selector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We may need to change some of the app var names then. I have a feeling some of the old ones are still there from Sickrage.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can do whatever we like inside the app, but we can't change the config.ini key name without a migration.
I'm fine with it for now, I was just stating a fact, to explain why it should be layout.anime and not anime.home.

@@ -342,6 +340,82 @@ def data_main():
section_data['indexers']['config'] = get_indexer_config()
section_data['processMethod'] = app.PROCESS_METHOD

section_data['freeMobile'] = NonEmptyDict()
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a different PR where we decided to make a notifications endpoint, didn't we?
Looks like I never opened the PR for it, the branch is bugfix/api-config-patch-multiple.
The commit: 1d66853

You can do this in this PR, instead of put these in data_main make a new function data_notifications and put them there, including kodi, plex and emby.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe i should do this in #4913

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's drop this PR and just move the needed ones to #4913

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay just to be clear. In #4913 i'm taking responsibility for moving the notifiers config (for now kodi, plex and emby) to data_notifications. But not the other changes of this PR?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, we can re-add these in another PR if still needed.

@OmgImAlexis OmgImAlexis modified the milestones: 0.2.7, 0.2.8 Jul 21, 2018
@p0psicles
Copy link
Contributor

@OmgImAlexis whats the status on this?

@OmgImAlexis OmgImAlexis modified the milestones: 0.2.9, 0.2.10 Aug 11, 2018
@OmgImAlexis OmgImAlexis modified the milestones: 0.2.10, backlog Aug 22, 2018
@OmgImAlexis OmgImAlexis added Invalid and removed Enhancement Needs review Needs testing Requires testing to make sure it's working as intended labels Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants