-
Notifications
You must be signed in to change notification settings - Fork 712
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
moving skeleton from kolibri_skeleton repo #4
Conversation
Current coverage is
|
Great start @benjaoming! I'm excited for Kolibri's codebase with these standards in place from the very beginning. |
I'm curious on why that is. Were you talking about the backport of Py3's config parser to Py2? |
Thanks for the link! Actually I tried googling for that without luck :) Upside is that ini files are so much easier to edit manually! Downside is the extra dependency, but with a much improved dependency management, maybe we don't have to worry about that. A JSON config file almost entirely rules out that a normal user can open the file with a text editor.. ? Adding to the TODO! |
Any particular reason for Travis over Circle? |
3. The pull request should work for Python 2.6, 2.7, and 3.3, and for PyPy. | ||
Check https://travis-ci.org/learningequality/kolibri | ||
under pull requests for active pull requests or run the ``tox`` command and | ||
make sure that the tests pass for all supported Python versions. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
None. It just came pre-configured and it's really easy to setup. Circle has the parallel containers, SauceConnect, and SSH access that we were using quite a lot, it's on the TODO list to make the switch. |
|
||
# When we start loading stuff from kolibri, we're gonna need this | ||
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "kolibri.deployment.default.settings.base") | ||
os.environ["KOLIBRI_HOME"] = tempfile.mkdtemp() |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Skeleton looks good, I'd like to scrutinize the plugin a little closer and get the core team's opinion. Regarding the above checklist, I feel like following items must be addressed before we merge this (and the rest we can open issues for):
|
class MandatoryPluginMethodNotImplemented(Exception): | ||
|
||
def __init__(self): | ||
NotImplemented.__init__(self, "Plugin needs to define this method") |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
One more important thing -- let's add a User Documentation stub. It will let users know we're thinking about them, even if it's quite bare for the time being. |
|
||
|
||
from kolibri.plugins import registry | ||
registry.initialize() |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
I've implemented logging so it's 100% configurable in django's settings.LOGGERS. Our default logging is thus specified in our default deployment, i.e. This includes two different log files: One for standard logging |
I propose finishing the discussion on the logging mechanism. Current status is:
The latter can be adjusted as we go along. But I've removed the |
Logging looks good. There are still unaddressed issues, but as I stated above I think we should merge and address them individually. The PR is getting too long. I'll open some, @rtibbles please open any I miss. |
moving skeleton from kolibri_skeleton repo
Adding an empty `.stylintrc` file
User creation - PR review updates
Updated APK buildkite script to more safely copy APK out of the image
Make "pip install -e ." work
EF2E-7 Added margin values to setup wizard stylesheet Approved-by: Daniel Juranec <[email protected]> Approved-by: Mayank Shah <[email protected]> Approved-by: Andrei Popescu <[email protected]>
multiple translators in a single file
NEWS
Anything you read here is a proposal, even though the sentence doesn't start with "I propose that ..." :)
The skeleton doesn't concern node.js or the likes. I simply don't have the skills, and I hope someone will jump in and make the most awesome skeleton for JS packages and Restful APIs shortly after the Python stuff is laid out.
TODO
A more universal TODO can be maintained here, as for now, this is the PR TODO which we may defer.
Whether to solve these now or after merge, is undecided ( @MCGallaspy ? )
tox -e bdd
kolibri-static
dist packaging mechanismWe have coverage with codecov.io
This will show detailed code coverage reports in all PRs. Before we start, we need to have it at 100%! :)
Sphinx integrates with our codebase
What happens is basically that we can start writing documentation directly in our source code. Certain chapters will need more careful explanation and can be written as separate files. Others can be separate but still import source code comments. Case in question is how we use the
docstring
fromkolibri.utils.cli
:http://kolibri.readthedocs.org/en/latest/usage.html
We no longer have a script that launches a different process that forks a daemon
We spent a lot of time with these issues. The problem was that
kalitectl.py
was called from a scriptbin/kalite
and then went on to invoke Django. Instead, now we have a conventionkolibri.__main__
module which will already have the correct path when called aspython -m kolibri [command line arguments]
. For installations, usingsetup.py
'sentry_point
option, we can make thekolibri
alias without placing any executables anywhere. As for Windows, things got slightly easier because even though the PATH for point at executables vary between different Windows environments, we can create a kolibri.bat that only needs to dopython.exe -m kolibri [args]
.The default django
runserver
command is also back, including the nifty auto-reloading feature:The docopt clutter is gone
Vanilla docopt is invoked, no more of the problems with CLI argument order. Explicitness FTW.
PEP8 checks will fail your PR
As for pep8, the current proposal is to be strict. Before the CI will even consider running the tests, it'll check that everything is pep8. If not, it breaks.
Static code analysis will fail your PR
More importantly perhaps, static code checks are enforced. As with pep8, these are common to have in an IDE and should prompt warnings. Typical examples from KA Lite counted:
Py2+3
All code is considered to be Python 2 + 3. We will probably need the
six
library a lot.Multi-env testing
Tox is used so the CI will run tests both in Python 3.4 and 2.7. Read instructions here:
http://kolibri.readthedocs.org/en/latest/#testing
Make is a nice to have (tox is where it happens)
There is a Makefil. But it's not mandatory for anything -- tox is on the other hand. But we should try to avoid Make for the sake of our Windows devs.
Kolibri settings layers
_Kolibri is technically speaking a collection of Django applications, with preconfigured Django projects that are invoked through the
kolibri
CLI._In order to make the Kolibri fly, we have two layers of configuration:
~/.kolibri/kolibri_settings.json
(NB! Whether or not it's JSON is totally up for debate, however I found ConfigParser totally incompatible betwee Py2 and Py3 and PyYaml to be a third-party with possible bindings).~/.kolibri/settings.py
, but let's see how far we can get with injecting values from the above file because it's simply easier to deal with a real settings API and not a crazy Python file :)Plugin API
The thing I've been trying to work out and will still add more methods and "hooks" for, is the plugin API. It's become quite versatile and flexible.
There were to paradigms that I've ended up mixing so they should have the best of both worlds.
But firstly, behold that plugins are enabled and disabled directly from command line:
Once the plugin is loaded, it will write itself into the kolibri settings file. If you run Django migrations, you now see that it's part of
INSTALLED_APPS
:Hooks
There is an example plugin that has the following code:
It currently yields this:
So where are the hooks called / rendered?
Anywhere! That's the beauty of it. Any core component or plugin can access
kolibri.plugins.hooks
and get a list of callables and use them in the context they desire. The important part is that we write down the intention of the hook and do not break this contract.Why a plugin class?
For extendibility! Even though plugins are django apps and that makes them a bit tricky to extend, we still want to be able to overwrite a plugin easily. Exactly how this will go down is yet to come, but a singular plugin class inheritance is the beginning.