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

Feature/show sub original release #7955

Merged
merged 19 commits into from
Apr 19, 2020
Merged

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented Apr 11, 2020

  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

As requested by @ratoaq2

Reminders

  • Update changelog
  • Update api spec
  • Do some real testing

This was already prepared for another PR, where it will be used by the snatch-selection (vue) component and the history (vue) component.
For this addition to the subtitle-search.vue component. Only one of the actions in history.js is used.
@p0psicles p0psicles marked this pull request as ready for review April 14, 2020 10:56
d = {}
d['id'] = item['rowid']

if item['indexer_id'] and item['showid']:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these really be missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For an episode it can't.
But i've been overthinking the decistion to create the episode_history.py route. (apiv2/history/tvdb12345/episode/s01e01). But maybe I should just use a parameter to specify an episode?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's consistent with the rest of the routes. But since they can't be missing for episode, we should raise an exception or leave the check out completely?


yield d

if not len(results):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think len() is needed here

d['id'] = item['rowid']

if item['indexer_id'] and item['showid']:
d['series'] = SeriesIdentifier.from_id(item['indexer_id'], item['showid']).slug
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it's required. As for example for the history.mako page, we just want to get all history information.

Copy link
Contributor

Choose a reason for hiding this comment

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

But still indexer_id and showid must be in the DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ow yeah good point


yield d

if not len(results):
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above


def data_generator():
"""Read log lines based on the specified criteria."""
start = arg_limit * (arg_page - 1) + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove the +1 here you can remove the two -1 below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, not sure. For ex. Page 1, with a Limit of 50. Will result in 1, page 2 will result in 51

Copy link
Contributor Author

@p0psicles p0psicles Apr 15, 2020

Choose a reason for hiding this comment

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

I've changed it for now.
It's not used anywhere right now. So I'll leave it at that.
When i'm picking up the vueify history page, i'll test it out.

"""Read log lines based on the specified criteria."""
start = arg_limit * (arg_page - 1) + 1

for item in results[start - 1:start - 1 + arg_limit]:
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are less results than limit? Possible list index out of range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's possible, as it's slicing the results

x-request:
path-params:
seriesid: tvdb999999999
x-disabled: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@medariox @sharkykh yes i'm disabling these.
I'm not getting it to work. If some of you want to spend time on this, feel free.

@medariox medariox merged commit 1a8dfb5 into develop Apr 19, 2020
@medariox medariox deleted the feature/show-sub-original-release branch April 19, 2020 11:10
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.

2 participants