-
Notifications
You must be signed in to change notification settings - Fork 38
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
global: module refactoring #18
Conversation
|
||
from invenio_base.globals import cfg | ||
from invenio_ext.sqlalchemy import db | ||
from invenio_utils.text import to_unicode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have to rely on invenio_utils
, is this OK or should I find equivalent method in some other available libs ? @lnielsen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use invenio utils. Perhaps you can use another library or simply copy the code into the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
six.text_type
should to the work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
six.text_type(foo)
seems to work. It does not support accents, but I think I can assume pid_value
and object_value
won't contain any.
47b9f1e
to
2ad98f0
Compare
@@ -1,15 +0,0 @@ | |||
.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be removed (i.e. use the one generated by cookiecutter). Probably missing to copy the dot files when overwriting the old code?
@kiryx Good job! It's not an easy task to jump onto a module like this as the first thing. Don't despair from the number of comments - it's all minor things and not easy to know. |
2308cd2
to
6f7e3e5
Compare
from werkzeug.utils import import_string | ||
from werkzeug.local import LocalProxy | ||
|
||
__all__ = ["pidproviders", ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__all__ = ('pidproviders', )
From the comments above what is still to be done:
|
b8ac26e
to
f91b979
Compare
* INCOMPATIBLE Refactoring and column name change for pidSTORE table. * Added unit tests for models and PID providers. Signed-off-by: Krzysztof Nowak <k.nowak@cern.ch>
This PR, addresses Issue #19