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

abstract data interface #43

Merged
merged 10 commits into from
Jun 25, 2017
Merged

abstract data interface #43

merged 10 commits into from
Jun 25, 2017

Conversation

gideonite
Copy link
Contributor

Abstract the interface for data storage from anything mongodb specific,
thereby allowing for developers to more easily incorporate other
backends such as file storage and tinydb.

Basic implementation for file storage with corresponding unit tests.

For more on this change see #17.

Abstract the interface for data storage from anything mongodb specific,
thereby allowing for developers to more easily incorporate other
backends such as file storage and tinydb.

Basic implementation for file storage with corresponding unit tests.
@coveralls
Copy link

Coverage Status

Coverage increased (+3.05%) to 74.05% when pulling 5ecdcf4 on gideonite:develop into 45e9b73 on chovanecm:develop.

Copy link
Owner

@chovanecm chovanecm left a comment

Choose a reason for hiding this comment

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

Nice! Could you please enable "allow edits from maintainers" for this branch so that I can push add minor changes before merging? (I want to add / restyle docstrings and fix some of the code style warnings reported by https://travis-ci.org/chovanecm/sacredboard/jobs/239662238 in the new code.)
Alternatively, you can do it :-) And I think it's acceptable to ignore warnings for the test files.

@@ -76,12 +79,20 @@ def run(debug, no_browser, m, mu, mc):
Note: MongoDB must be listening on localhost.

"""
add_mongo_config(app, m, mu, mc)

if m:
Copy link
Owner

Choose a reason for hiding this comment

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

Please include the mu variable too in order for commands like sacredboard -mu mongodb://user:pwd@host/admin?authMechanism=SCRAM-SHA-1 sacred to work.

@gideonite
Copy link
Contributor Author

gideonite commented Jun 6, 2017

I'd be more than happy to but I don't know how to run the linter tool.

I'm pretty sure allow edits from maintainers is already on:

image

@coveralls
Copy link

Coverage Status

Coverage increased (+3.05%) to 74.05% when pulling ecc2caa on gideonite:develop into 45e9b73 on chovanecm:develop.

@chovanecm
Copy link
Owner

chovanecm commented Jun 6, 2017

If you still have troubles running tox (e.g. tox -e flake8), the commands for running it are defined in tox.ini.
So, for instance, flake8 sacredboard --max-complexity 10 --exclude tests checks the code style and pydocstyle sacredboard --ignore D207,D212,D301,D203 the Python code documentation. This is known to work on Windows.
Tox, unfortunately, doesn't run the second command if the first one fails, so it doesn't say anything about your documentation style if there is a problem with code style. I have to resolve it somehow, perhaps I will have to add a Makefile, but it won't work for most users on Windows either.

Also make sure to have both flake8 and pydocstyle installed (I just noticed I forgot to add them in dev-requirements.txt, so if you download the current version and run pip install -r dev-requirements.txt, it should install automatically, or simply run pip install flake8 pydocstyle).

@gideonite
Copy link
Contributor Author

Whoops, I guess I should have pulled before I pushed. Should I pull and resolve the changes or leave that to you? The only changes I made to mongodb in this last commit relate to removing whitespace and shortening doc strings as recommended by flake8.

@chovanecm
Copy link
Owner

Conflict fixed. I have also fixed some errors and added reading of the info.json file.

Furthermore, I changed the way how the code style check for Python works.
Now, tox -e flake8 checks the code style whereas tox -e pydocstyle is responsible for code documentation checks. The reason for it is that when flake8 found errors, pydocstyle did not run at all.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.5%) to 74.513% when pulling c8f3c88 on gideonite:develop into 53ffcb4 on chovanecm:develop.

@chovanecm
Copy link
Owner

Now I'd like to ask you to consult the code documentation with tox -e pydocstyle (of course, only files that are relevant to your part).

Do you plan to add support for filtering, sorting etc? It's quite a lot of work. Otherwise, I'm thinking about extending the Web API to tell the frontend to hide the filtering and similar controls in order not to confuse the users.
Also, some error handling (file not found) etc would be nice. But for that, I have to modify the Web API.

The run "info" is normally stored in an external file. But it was not so in the test run.
@coveralls
Copy link

Coverage Status

Coverage increased (+3.5%) to 74.513% when pulling ec56339 on gideonite:develop into 53ffcb4 on chovanecm:develop.

@chovanecm chovanecm added this to In progress in Sacredboard - implementation Jun 9, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+3.3%) to 74.313% when pulling 804b563 on gideonite:develop into 53ffcb4 on chovanecm:develop.

@chovanecm
Copy link
Owner

chovanecm commented Jun 11, 2017

"Why is m or mu compared to (None, None)?" - It's not exactly clear, I know. The reason is that the mu parameter defaults to (None, None) and when the parameter is not present on the command line, it is set to that value. So after you added "if m or mu", it was actually always evaluated as True even thought Sacredboard was run with -F.
I have to refactor the bootstrap. It's getting a little bit messy.

@gideonite
Copy link
Contributor Author

Oh ok, sorry about that. I should have tested it more thoroughly.

Any other things that this PR is blocked on?

@chovanecm
Copy link
Owner

Not from your side. As soon as I have some time (hopefully already later this week), I'll disable all the sorting and filtering controls for backends that don't support it and I'll merge the PR.

@chovanecm chovanecm moved this from In progress to Blocked in Sacredboard - implementation Jun 12, 2017
@chovanecm chovanecm merged commit 804b563 into chovanecm:develop Jun 25, 2017
@chovanecm chovanecm moved this from Blocked to To be tested / validated in Sacredboard - implementation Jun 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Sacredboard - implementation
To be tested / validated
Development

Successfully merging this pull request may close these issues.

None yet

3 participants