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

[script.simkl] 2.2.0 #1190

Merged
merged 1 commit into from Dec 2, 2019
Merged

[script.simkl] 2.2.0 #1190

merged 1 commit into from Dec 2, 2019

Conversation

ekleop
Copy link

@ekleop ekleop commented Oct 21, 2019

Description

Checklist:

  • My code follows the add-on rules and piracy stance of this project.
  • I have read the CONTRIBUTING document
  • Each add-on submission should be a single commit with using the following style: [script.foo.bar] 1.0.0

Additional information :

  • Submitting your add-on to this specific branch makes it available to any Kodi version equal or higher than the branch name with the applicable Kodi dependencies limits.
  • add-on development wiki page.
  • Kodi pydocs provide information about the Python API
  • PEP8 codingstyle which is considered best practise but not mandatory.
  • This add-on repository has automated code guideline check which could help you improve your coding. You can find the results of these check at Codacy. You can create your own account as well to continuously monitor your python coding before submitting to repo.
  • Development questions can be asked in the add-on development section on the Kodi forum.

@TravisBuddy
Copy link

Travis Buddy

Hey Ichika,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@ekleop
Copy link
Author

ekleop commented Nov 14, 2019

Hi @Rechi is there an approximate timeline when this will be merged?

@@ -24,5 +24,9 @@
<fanart>resources/fanart.jpg</fanart>
</assets>
<language>en</language>
<news>
Copy link
Member

Choose a reason for hiding this comment

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

the news tag is only available for krypton and later versions

Copy link
Member

Choose a reason for hiding this comment

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

Although I am ok with keeping it in the addon.xml since the tag will be ignored by jarvis

Copy link
Author

Choose a reason for hiding this comment

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

Doesn't krypton and later versions cannot install the addon? so they will use this tag or am I wrong?

Copy link
Member

Choose a reason for hiding this comment

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

This add-on can be installed in Krypton and newer versions. Those versions do not have a changelog.txt fallback, so if you want to see a changelog inside Kodi, you have to keep the news tag.

Copy link
Author

Choose a reason for hiding this comment

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

yeap, that is what I wanted to know. We left it specifically for this. Thanks.

@@ -0,0 +1,4 @@
files:
Copy link
Member

Choose a reason for hiding this comment

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

please keep this file out of the submission

Copy link
Member

@enen92 enen92 left a comment

Choose a reason for hiding this comment

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

Only the yaml file that needs to be removed from the submission. Apart from that, it is good to go.
Sorry for the huge delay reviewing the addon.

@enen92
Copy link
Member

enen92 commented Nov 22, 2019

@ekleop please also drop all the .po files without translations. There's no point in having those since Kodi will always fallback to the en as the default lang

@TravisBuddy
Copy link

Travis Buddy

Hey Ichika,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@ekleop
Copy link
Author

ekleop commented Nov 23, 2019

ok, I've removed yaml file + all empty language files.Everything should be fine now.

@ekleop ekleop requested a review from enen92 November 23, 2019 21:40
@KarellenX
Copy link
Member

@enen92
Could you submit this if all is ok. It has been over 5 weeks since the PR was submitted.

Thanks

@enen92 enen92 merged commit 643a431 into xbmc:jarvis Dec 2, 2019
@KarellenX
Copy link
Member

Thanks @enen92

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.

None yet

5 participants