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
[Init] Initial works to split the json schema widget into a package. #1
Conversation
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.
travis config, tests and some clean up needed probably.
@@ -0,0 +1,117 @@ | |||
a.button:focus, | |||
a.jsoneditor-exit:focus{ text-decoration: none } |
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.
may be this should be dropped
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.
if this code is imported from the current implementation we can leave it as is, we can deal with the UI in the next step
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 agree
@nemesisdesign could you check if this is nearby for initial merge and manual testing? some minor tweaks may be needed, I will start work on test after this is in |
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.
great progress @auvipy 👍 see my comments
@@ -0,0 +1,117 @@ | |||
a.button:focus, | |||
a.jsoneditor-exit:focus{ text-decoration: none } |
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.
if this code is imported from the current implementation we can leave it as is, we can deal with the UI in the next step
probably I have addressed the comments. except for sphinx. do you think this pr is ok to get in? |
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.
@auvipy ready for manual testing, I will test as soon as possible and added this to my TODO list.
I left some comments regarding minor improvements.
{% endblock javascript %} | ||
|
||
{% endblock extra_js %} | ||
{% endcomment %} |
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 don't get the point of this file
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.
will take care of this after your manual testing. other comments are addressed hopefully
Will get here asap. |
OK. Looking forward to it |
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 tested it out openwisp/django-netjsonconfig#96 ; first project to use this repository looks like working ! 👍
Note: I had to make changes for Django-2.1 compatibility: https://github.com/atb00ker/django-jsonschema-widget
I have a couple of questions / comments:
- Should we start with automated testing? 😄
(please read more questions below)
jsonschema_widget/widgets.py
Outdated
</label> | ||
""" | ||
html = html.format(_('Advanced mode (raw JSON)'), | ||
reverse('netjsonconfig:schema')) |
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.
- Since we are moving it out of netjsonconfig, my assumption is that it's planned to be used at other places as well!
Hence, shouldn't all these values be either variables (to be set from the settings file) or changed and made generalised to keep any project in mind!
I am not only talking about thereverse
here, but also values above are keeping only that one project in mind!
Please correct me if i've understood it incorrectly! 😄
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 not sure right now, @nemesisdesign might have better insight than me in this regard.
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.
added Django 2.1 related changes. will start working on automated testing after this get merged initially. for automated test, which approach or tools you prefer? |
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 found Issues during manual testing on django-netjsonconfig and reported to openwisp/django-netjsonconfig#96 (review)
I propose the following: let's fix the easiest stuff (namings and conventions) and leave the harder stuff for more iterations, let's merge, I'm adding both of you to have write permissions to the repo so we can work on it together here.
We can also create some GCI tasks for improvements like using Pipenv
and similar, while we focus on making the package work and get it to the point that we can issue the first release and we can merge openwisp/django-netjsonconfig#96
thanks for your review, Will fix the easier tasks first and after that will merge and start more iteration for the next concerns. |
Great, thanks. I enabled also travis and coveralls |
P.S: Also, the commit guidelines of the rest of the projects for consistency! 😊 |
should I disable jslint check for now? or fix them all? |
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.
OK |
what's the reason for build failures? any hints for fixing it? |
I want to get this in and start working on automated testing |
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.
@auvipy thank you for pinging us, we've been super busy with the Google Code-In which is almost over now.
I left you some comments that should help you resolve the situation. Once you have done please squash the commits into one.
.travis.yml
Outdated
|
||
script: | ||
- coverage run --source= runtests.py | ||
- if [[ $TRAVIS_PYTHON_VERSION == 3.4 ]]; then ./tests/manage.py makemigrations --dry-run | grep "No changes detected"; fi |
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.
you can also remove the migration check because we don't have any model
I will address all the issues tomorrow. |
I might have messed up something in shell section, could any of you take another look? do we need to add django as a dependency on requirements-test? |
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.
@auvipy is just the commit message style check doing its job, we added a check in all our modules to ensure the commit message style used is consistent so that the commit log of each project is readable
Could you please clean up the commit history of this PR so we can merge?
The fastest way is to squash all the previous commits into one and give it a message like:
[init] First commit
I must have to do this before this week ends!!! |
Great, thanks! 👍 |
No description provided.