Skip to content

Change trakt URLs to TLS and plural collections.#1760

Merged
cvium merged 1 commit intoFlexget:developfrom
haarts:update-trakt-human-site-to-https
Apr 6, 2017
Merged

Change trakt URLs to TLS and plural collections.#1760
cvium merged 1 commit intoFlexget:developfrom
haarts:update-trakt-human-site-to-https

Conversation

@haarts
Copy link
Copy Markdown
Contributor

@haarts haarts commented Mar 28, 2017

Motivation for changes:

The URLs generate by flexget in various places still used http as
protocol. This was changed by Trakt to https some time ago. The commit
reflects that fact.

Several URLs have changed from using serie and movie in the path to
their plural form. Trakt.tv redirected these singular forms to the
canonical plural form.

Detailed changes:

In essence this was a search and replace exercise.

Addressed issues:

None. All the 'wrong' URLs are correctly redirected by Trakt.tv. They might stop doing that at some point.

The URLs generate by flexget in various places still used http as
protocol. This was changed by Trakt to https some time ago. The commit
reflects that fact.

Several URLs have changed from using `serie` and `movie` in the path to
their plural form. Trakt.tv redirected these singular forms to the
canonical plural form.
item['show']['ids']['slug'], item['episode']['season'], item['episode']['number'])
else:
entry['url'] = 'http://trakt.tv/%s/%s' % (list_type, item[list_type]['ids'].get('slug'))
entry['url'] = 'https://trakt.tv/%ss/%s' % (list_type, item[list_type]['ids'].get('slug'))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

%ss?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A couple of lines up list_type is defined of which the trailing s is stripped, yielding 'serie'/'movie'/'episode'. That variable is use as the key to the Trakt API response dictionary. And it is also used here to construct the URL. Which requires a trailing 's' again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't it more easy to then just drop the stripping of the s and change those other locations where it is compared?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mfonville well... The variable list_type is used in two ways. One, and the most prevalent, is as the index or as the comparison to a dictionary index. The other, used only once, as part of this URL generation.
I've opted to only make the URL generation slightly off and keep the majority the same.

@cvium
Copy link
Copy Markdown
Contributor

cvium commented Mar 28, 2017

Several URLs

What URLs? I don't think it makes sense to make partial changes right now if they're going to change things again soon.

@haarts
Copy link
Copy Markdown
Contributor Author

haarts commented Mar 28, 2017

@cvium I have no reason to believe the Trakt operators are going to change anything anymore. I'm sorry if I gave that impression. This commit encompasses a comprehensive change.

The URLs changed are:

  • every 'http' became 'https'
  • all paths '/movie/' and '/show/' became '/movies/' and '/series/'

@haarts
Copy link
Copy Markdown
Contributor Author

haarts commented Apr 6, 2017

Is there anything I can do to move this PR forward?

@liiight
Copy link
Copy Markdown
Member

liiight commented Apr 6, 2017

convince @cvium to merge it

@haarts
Copy link
Copy Markdown
Contributor Author

haarts commented Apr 6, 2017

I'll give that a go then!
@cvium does my explanation address your question?

@cvium cvium merged commit ad2dfa3 into Flexget:develop Apr 6, 2017
@haarts haarts deleted the update-trakt-human-site-to-https branch April 6, 2017 18:07
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.

4 participants