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

Add "enum" for countries #52

Closed
wants to merge 6 commits into from
Closed

Add "enum" for countries #52

wants to merge 6 commits into from

Conversation

@zachantre
Copy link
Contributor

@zachantre zachantre commented Aug 20, 2016

Fixed timezone in openwrt backend
Addition of countries and selection from menu

home
Addition of countries and selection from menu
@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Aug 23, 2016

Hi @zachantre, thanks for your contribution.
There's one thing I don't understand, what exactly do you want to fix in the timezone setting?

@zachantre
Copy link
Contributor Author

@zachantre zachantre commented Aug 25, 2016

Well, openwrt timezone need time format... right now it submits continent/city which is not proper for openwrt. With fix, on site you see continent/city but submit the actual timezone

@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Aug 26, 2016

That shouldn't be an issue because the library supplies the correct value upon rendering/generation.

But I would like to accept your patch in order to address the mismatch so the implementation would become cleaner, I'm trying your patch but lots of things go wrong.

Have you tried running unit tests as explained in the README?

Federico

@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Aug 26, 2016

I see many failing tests with an error which I think it's hard to overcome: OpenWRT has quite a few timezones of different areas which have the same value, but specifying an enum which has duplicate values is not allowed by jsonschema RFC.

Example:

    (
        "Africa/Gaborone",
        "CAT-2"
    ),
    (
        "Africa/Harare",
        "CAT-2"
    ),

Supplying a list which has duplicate values to enum breaks the schema validation.

To understand what I'm trying to say, run this command:

nosetests -x tests.openwrt.test_backend

-x means stop at the first failure (otherwise you would get a ton of errors and you would be overwhelmed.

The other change to countries should easier to include.

@@ -782,6 +783,9 @@
"country": {
"type": "string",
"maxLength": 2,
"default": "IT",

This comment has been minimized.

@nemesisdesign

nemesisdesign Aug 26, 2016
Member

remove this line please, there's no reason for which the default country should be Italy ;-)

@zachantre
Copy link
Contributor Author

@zachantre zachantre commented Sep 5, 2016

I see... Sorry for my mistake, I have just started learning python.
I will fix it and recommit.
I like openwisp2 project very much, it's very innovative and have much space for a lot of features.

zachantre added 3 commits Sep 5, 2016
@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Sep 5, 2016

Great! We just need to cleanup a few style issues before I can merge your patch.

@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Sep 5, 2016

See the failing build:
https://travis-ci.org/openwisp/netjsonconfig/jobs/157587560

You just have to remove a few trailing spaces from the indicated lines.

Then when you are done, do the following to squash your commits:

git rebase -i HEAD~4

Leave the last commit untouched and set the previous ones as "squash", then provide a better commit message and save, then push to your repository using git push --force and we are done.

Thank you @zachantre!

zachantre added 2 commits Sep 5, 2016
@coveralls
Copy link

@coveralls coveralls commented Sep 5, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling f0bd46d on zachantre:master into 8d66f7a on openwisp:master.

@zachantre
Copy link
Contributor Author

@zachantre zachantre commented Sep 5, 2016

On my pc I had some issues with tests... not properly set virtual env...
Anyway, after some changes, travis shows everything fine... :)
I also removed timezones enum as suggested since the library supplies the correct value upon rendering

@nemesisdesign nemesisdesign changed the title some little fixes with my little knowledge Add "enum" for countries Sep 5, 2016
nemesisdesign added a commit that referenced this pull request Sep 5, 2016
@nemesisdesign
Copy link
Member

@nemesisdesign nemesisdesign commented Sep 5, 2016

Thank you @zachantre, I squashed the commits and merged manually in d23d8fb. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants