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

Issue77 #78

Merged
merged 12 commits into from
Dec 4, 2020
Merged

Issue77 #78

merged 12 commits into from
Dec 4, 2020

Conversation

itsdkey
Copy link
Contributor

@itsdkey itsdkey commented May 9, 2020

So what is done here:

  1. I've restructured the project so it is a normal Django project
  2. I've added an example database (I still didn't add any content_type examples)
  3. I've fixed the problem with creating a model only for tests (using an approach in the link
  4. I've rewritten the tox.ini file to support:
    a) python3.6 - python3.8
    b) django2.2 - django3.0 (because these versions are currently supported)
    c) travis (I used the existing config, only changed the python version to 3.8)
    d) docs (I used the existing config, only changed the python version to 3.8)

What still needs to be done:

  • 1. project refactor (Checkout the code, see if anything can be improved or thrown away because of dropping support for python < 3.6)
  • 2. documentation checkout
  • 3. check how things work when we install this as a package
  • 4. update readme with supported versions
  • 5. add tests if necessary (which can improve coverage to the 90% milestone)

Things to consider:

  • Add factory-boy -- it can help improving tests by using factories. I personally love this package, I've been using it for like 4-5years.

@itsdkey
Copy link
Contributor Author

itsdkey commented May 9, 2020

Hey guys,
I've done some work on the major update. Tests are running on tox so the project is supported from py36 to py38. You can download it and check it by yourself 😃

If you have any suggestions, please drop a comment.

@coveralls
Copy link

coveralls commented May 9, 2020

Coverage Status

Coverage decreased (-0.2%) to 89.593% when pulling 81e1880 on itsdkey:issue77 into 113962c on caktus:develop.

Copy link
Contributor

@vkurup vkurup left a comment

Choose a reason for hiding this comment

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

Sorry, again, for the delay. I haven't had a chance to run the sample app yet, but these changes look great to me. I agree with the checkboxes you've set out in your PR description. We also love FactoryBoy, so yes, feel free to change tests to use that if it makes things easier/clearer.

Thanks again for taking on this work! 💯

setup.py Show resolved Hide resolved
setup.py Outdated
'Topic :: Software Development :: Libraries :: Python Modules',
'Development Status :: 5 - Production/Stable',
'Operating System :: OS Independent',
],
long_description=open('README.rst').read(),
install_requires=[
"django-mptt>=0.8.6,<1.0",
"django-mptt>=0.11.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to add ,<1.0 here, in case a future major version has backwards incompatible changes.

treenav/apps.py Show resolved Hide resolved
@@ -10,9 +10,17 @@
register,
show_treenav,
single_level_menu,
CaktNode,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like flake8 says this is an unused import, so can be removed.

@itsdkey
Copy link
Contributor Author

itsdkey commented Aug 13, 2020

I've corrected the code that you've pointed out. I'm not sure why coveralls doesn't work.

setup.py Outdated Show resolved Hide resolved
@vkurup vkurup merged commit c4e88a5 into caktus:develop Dec 4, 2020
@vkurup
Copy link
Contributor

vkurup commented Dec 4, 2020

Thank you @itsdkey! I also am not sure why coveralls is complaining, but this looks great to me!

@itsdkey
Copy link
Contributor Author

itsdkey commented Dec 6, 2020

Sure, no problem 👍

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

Successfully merging this pull request may close these issues.

3 participants