[Trivia] Move Trivia lists back home #2028

Tobotimus · 2018-08-18T13:02:22Z

This still needs discussion, but this means saying no to namespace packages (yay) and also everything I listed on #1973. Discussion on that issue still needs to occur, but I want to get it rolling and make a decision on this soon.

What I've done is move the trivia lists to the redbot/cogs/trivia/data/lists directory. The reason I chose and extra data sub-directory is for the interest of ease of extensibility, the distribution will now include all files in an existing data sub-directory of a package.

This also fixes our issue (which has only just been recently discovered) that locale files haven't been included with the distribution since beta 9, due to a line added in #1420: include_package_data=True. This reverts that change and also greatly simplifies the data file discovery for the distribution. I'm for more maintainable and straightforward code where possible.

Removes red-trivia as a dependency. Signed-off-by: Toby Harradine <tobyharradine@gmail.com>

Signed-off-by: Toby Harradine <tobyharradine@gmail.com>

* The distribution will now include all files under any data/ sub-directory of a package, as well as all *.po files under any locales/ sub-directory (as it should have been before). * MANIFEST.in has been simplified to comply with these changes and redbot/cogs/audio/application.yml has been moved to the data/ sub-directory to maintain consistency in how we declare package data. Signed-off-by: Toby Harradine <tobyharradine@gmail.com>

Tobotimus

Here are just some explanatory comments.

Tobotimus · 2018-08-18T13:05:18Z

Pipfile.lock

@@ -53,6 +53,7 @@
                "sha256:474d4bc64cee20603e225eb1ece15e248962958b45a3648a9f5cc29e827a610c",


Just a comment: The Pipfile.lock was simply updated as Red-Trivia is no longer a dependency. For some reason it has also included some extra markers - these have no effect so please disregard.

Tobotimus · 2018-08-18T13:08:02Z

MANIFEST.in

@@ -1,5 +1,3 @@
 include README.rst
 include LICENSE
 include dependency_links.txt
-include discord/bin/*.dll
-include redbot/cogs/audio/application.yml


The discord/bin/*.dll files don't need to be in our manifest as they're not part of our distribution.

As for the redbot/cogs/audio/application.yml file, I've moved it to redbot/cogs/audio/data/application.yml as we're now including everything in the data/ sub-directory of all packages if it exists (much like we do with 3rd party cogs). This reduces boilerplate and maintains consistency.

Tobotimus · 2018-08-18T13:10:24Z

setup.py

@@ -35,11 +33,6 @@ def get_dependency_links():
        return file.read().splitlines()


-def get_package_list():
-    core = find_packages(include=["redbot", "redbot.*"])
-    return core


Since this is literally just aliasing a single function call, I removed the function and placed the raw one directly in the setup function for clarity.

Tobotimus · 2018-08-18T13:12:59Z

setup.py

-    Ignore this tomfoolery in the desire for automation. It works, that's
-    all you gotta know. Don't fuck with this unless you really know what
-    you're doing, otherwise we lose all translations.
-    """


So this might be your biggest question, why did I remove this function: it's over-complicated, doesn't encourage extensibility of our data files and objects to maintainability. It is achieved in a much simpler manner with the usage explained below.

Tobotimus · 2018-08-18T13:17:19Z

setup.py

@@ -103,9 +67,8 @@ def glob_locale_files(path: Path):
    setup(
        name="Red-DiscordBot",
        version=get_version(),
-        packages=get_package_list(),
-        package_data=find_locale_folders(),
-        include_package_data=True,


include_package_data is a very misleading argument which we should never use in the interest of remaining sane. It actually forces setuptools to ignore the package_data argument. Please, for the sake of any future maintainers' sanity, don't bring it back (It's not clearly documented in setuptools' documentation, if you're unconvinced that this is the behaviour, test it on a local machine).

The new package_data argument is used in the way which is recommended in setuptools' documentation, where an empty string as a key applies the rule to all included packages.

Tobotimus · 2018-08-18T13:17:52Z

tests/cogs/test_trivia.py

+    from redbot.cogs.trivia import get_core_lists
+
+    list_names = get_core_lists()
+    assert list_names


This test came straight from the Red-Trivia repo.

mikeshardmind · 2018-08-18T23:51:53Z

I think I'd prefer trivia be loaded in as a submodule (still not a namespace package) here for a few practical reasons. The trivia lists don't seem to need to be under the same level of QA scrutiny as the rest of the project. They need to be kept accurate, but there aren't code paths in the questions. Leaving them in their current repo, but pulled in as a submodule allows the more lax review process for those, while gaining the other benefits.

calebj · 2018-08-19T00:22:04Z

+1 to what @mikeshardmind said. IMO, trivia lists shouldn't be in the main repo.

Tobotimus · 2018-08-19T03:21:11Z

@mikeshardmind @calebj If the only incentive to keep them out is to bypass QA, that can easily be done by us devs simply not assigning QA members to trivia lists PRs. Besides, as we know, @MiniJennJenn manages the trivia lists, is the only one who makes PRs to add trivia lists, and those trivia lists come from #12. I don't see how this is worth separating the repos or overengineering the solution to this "problem".

Finally, does this argument stack up against those I made in #1973? If you have disagreements with what I've stated there, you're welcome to point them out.

Tobotimus · 2018-08-19T05:59:39Z

I should probably think again before responding. For some reason I wasn't even thinking about git submodules, personally I've never used them but I understand now.

This would flatten the versioning system and it would keep them in the Red-DiscordBot distribution, which are both good things and solve two of the problems on #1973. The 3rd problem (neglect and separation) I could resolve by simply linking the ZenHub boards for the two repos (although I'm not convinced why we should separate the repos in the first place, esp. if we're only going to link their issue/PR boards back up again).

I'll look into using submodules. Thanks for your suggestions guys.

Tobotimus · 2018-08-19T07:08:31Z

After a bit of research and testing, I've decided against the proposition of using submodules. These are my reasons:

Cloning submodules includes everything which would be located in that submodule, as shown in the following image:

This means we'd have to be more specific in our globs for package_data in setup.py - part of this PR was to make that more general to simplify the process of adding data files in the future.
(the main reason) When the submodule is added and pushed to this main repo, it points to a specific commit on the submodule's repo, so if there any updates to the trivia lists we'd like to include in the next release, we'd have to open a new PR over here to update them. This is annoying and is a similar problem to the one we have now of maintaining separate release cycles on PyPI, and having to update the version pin on this repo every time Red-Trivia makes a new release.

So for those reasons I'm sticking to this approach here.

mikeshardmind · 2018-08-19T15:02:30Z

While that's correct, we don't actually need anything but the yml files to exist in that repo if we go that route. I'm still going to advocate the submodule route due to this, but I'm not strictly opposed to simply moving it all back here.

Tobotimus · 2018-08-19T15:26:43Z

We'd need the tests and Travis config in there at the least since we're not removing the CI on the lists. Besides, that wasn't my main gripe, it was more having to update the head of the submodule every time the trivia lists change.

mikeshardmind · 2018-08-19T17:06:57Z

Right... Just move them back then.

Tobotimus · 2018-08-24T14:00:56Z

Despite the mixed feelings about this I'm putting it into beta 20 as a possibly temporary fix for the namespace package issues. It's also a permanent fix for all of our missing translations, and that certainly needs to be fixed for beta 20. Personally I think the trivia files are going to stay where they are regardless.

Tobotimus added 3 commits August 18, 2018 01:38

[Trivia] Move trivia lists back home

576fdbe

Removes red-trivia as a dependency. Signed-off-by: Toby Harradine <tobyharradine@gmail.com>

Include package data in distribution

49bbcf9

Signed-off-by: Toby Harradine <tobyharradine@gmail.com>

Tobotimus added V3 Status: Needs Discussion Needs more discussion. labels Aug 18, 2018

Tobotimus requested review from palmtree5, tekulvw, Twentysix26 and Kowlin August 18, 2018 13:02

Tobotimus commented Aug 18, 2018

View reviewed changes

Tobotimus added Type: Fix and removed Status: Needs Discussion Needs more discussion. labels Aug 21, 2018

Tobotimus added this to the Beta 20 milestone Aug 23, 2018

Tobotimus added QA: Bypassed QA is unnecessary. and removed QA: Unassigned labels Aug 24, 2018

Kowlin approved these changes Aug 26, 2018

View reviewed changes

Merge branch 'V3/develop' into V3/vendor_trivia

5f36b45

Tobotimus merged commit e6495bc into Cog-Creators:V3/develop Aug 26, 2018

Tobotimus deleted the V3/vendor_trivia branch August 26, 2018 13:39

Tobotimus mentioned this pull request Aug 26, 2018

[V3] The trivia list repository and namespace packages #1973

Closed

Jackenmen added the Type: Bug Unexpected behavior, result, or exception. In case of PRs, it is a fix for the foregoing. label Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Trivia] Move Trivia lists back home #2028

[Trivia] Move Trivia lists back home #2028

Tobotimus commented Aug 18, 2018 •

edited

Loading

Tobotimus left a comment

Tobotimus Aug 18, 2018

Tobotimus Aug 18, 2018 •

edited

Loading

Tobotimus Aug 18, 2018

Tobotimus Aug 18, 2018

Tobotimus Aug 18, 2018

Tobotimus Aug 18, 2018

mikeshardmind commented Aug 18, 2018

calebj commented Aug 19, 2018

Tobotimus commented Aug 19, 2018

Tobotimus commented Aug 19, 2018 •

edited

Loading

Tobotimus commented Aug 19, 2018

mikeshardmind commented Aug 19, 2018

Tobotimus commented Aug 19, 2018

mikeshardmind commented Aug 19, 2018

Tobotimus commented Aug 24, 2018

		@@ -53,6 +53,7 @@
		"sha256:474d4bc64cee20603e225eb1ece15e248962958b45a3648a9f5cc29e827a610c",

[Trivia] Move Trivia lists back home #2028

[Trivia] Move Trivia lists back home #2028

Conversation

Tobotimus commented Aug 18, 2018 • edited Loading

Tobotimus left a comment

Choose a reason for hiding this comment

Tobotimus Aug 18, 2018

Choose a reason for hiding this comment

Tobotimus Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

Tobotimus Aug 18, 2018

Choose a reason for hiding this comment

Tobotimus Aug 18, 2018

Choose a reason for hiding this comment

Tobotimus Aug 18, 2018

Choose a reason for hiding this comment

Tobotimus Aug 18, 2018

Choose a reason for hiding this comment

mikeshardmind commented Aug 18, 2018

calebj commented Aug 19, 2018

Tobotimus commented Aug 19, 2018

Tobotimus commented Aug 19, 2018 • edited Loading

Tobotimus commented Aug 19, 2018

mikeshardmind commented Aug 19, 2018

Tobotimus commented Aug 19, 2018

mikeshardmind commented Aug 19, 2018

Tobotimus commented Aug 24, 2018

Tobotimus commented Aug 18, 2018 •

edited

Loading

Tobotimus Aug 18, 2018 •

edited

Loading

Tobotimus commented Aug 19, 2018 •

edited

Loading