Conversation
.travis.yml
Outdated
- RELEASE="0.1.0" | ||
- PROJECT="oca-decorators" | ||
- ODOO="10.0" | ||
- secure: "N4FxLhiQYktu5TUym48Kf5GoCeqSn/iNZT4rK9ktN18gOazkj5P9tPz33p8vde06f+yDTw6ETOZaTHUv7oGwlpc58ToMFey+XxlSyCeGoNVGi7uojs5DqF48lfKZbbTjCiiyj/6Ub7FH3IzcizbN+9WzJ6JB7v8U6eX8LwAW7YKPhdPctFM7rFZiLQp/r8lf0xN5WpMhufg3xloBJkS+iuQopULK7Il2g6Ag8Qo54IUFJzWQbZYKN8RMy0Ek41rgx88sKrTp9TDo0RBircy7f/6WH/CERTGRYtHCQDF0RToYaH9qiRiZ3w6vaxepbIicJCMCMvOQaSJ8IibIwBstwpkM0P2dHVP9WQiwxGgYMeCxS2MNkacQLAfxGXcMiRlfliq7SQpBYp7t3uEpby1l1UqDyTb+5aNUF8Y5CFx1hbbr2r++U8vLRxxvQRAzfl48Dy1jWw5N1hug/1VbGU7XuWPHiZd2WRngs2r7kUJVig9b3jQoi87kIr0V8FHjRALfsfo/swPD5mwI00qQ2eOySTKC8SSCToudByub1DYAPvuLn0JDGH1z7pDIe4rucZSSztvkiCOtlndYcm2hbyNLLVUN5Fn/GCmNZp2yer/h5Myn2kw3bMjdDLkVcgK5Ku2/eb5IMUWrjL4MbdczsUeqfNRK9oy5HEAAh+4sfre5Dw0=" |
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.
Generate a token for the OCA PyPi account
Also Travis needs secure variable for a Github token to post gh-pages
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.
No test, appreciate feature
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.
❤️
README.rst
Outdated
:target: https://codeclimate.com/github/OCA/oca-decorators | ||
.. |License MIT| image:: https://img.shields.io/github/license/OCA/oca-decorators.svg | ||
:target: https://opensource.org/licenses/MIT | ||
:alt: License: MIT |
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.
LGPL
setup.py
Outdated
|
||
from os import environ, path | ||
|
||
PROJECT = 'oca-decorators' |
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'd love it if we change the name of this, now that it's not yet too late. Calling it oca-decorators
seems to be too narrowing for possible helpers that are not decorators (context managers, test classes, data loaders, mock helpers, etc.).
I'd just call this ocalib
(or ocahelpers
, odoohelpers
, oca
or similar). The code would result into:
from ocalib import decorators
...
@decorators.foreach()
def what(self):
...
We could even decide to provide decorators in the main namespace, although this is not so important:
import ocalib
@ocalib.foreach()
def lalala()...
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.
+1 for wide name than only decorator
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.
setup.py
Outdated
version = environ.get('RELEASE') or environ.get('VERSION') or '0.0.1' | ||
|
||
if environ.get('TRAVIS_BUILD_NUMBER'): | ||
version += 'b%s' % environ.get('TRAVIS_BUILD_NUMBER') |
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.
Please don't hardcode travis. I already hit it with MQT. It's better IMHO if you provide a vendorless variable and transform the travis one into that in .travis.yml
.
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 need something to guarantee an increasing build number. Open to ideas, but this is the one I have.
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.
It's worth noting this works perfectly fine without Travis. It's required to push to PyPi with guaranteed versions. I've never actually been able to get tox
to work, so this is what we have.
I'm pretty sure this logic can be removed if someone submits a PR implementing tox
. I've spent way too much time trying to figure it out in the past, with zero luck, so I'm not willing to look again. Sorry, but I literally designed a whole build/test system because of my inherent inability to use tox
.
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.
@lasley The best way is to use setuptools_scm
to manage the version. It's what we use for git-aggregator for exemple. acsone/git-aggregator@545a097#diff-2eeaed663bd0d25b7e608891384b7298
As you can see in the previous link, you can also use travis to deploy the lib each time you create a tag.
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.
Thanks for the example @lmignon - much easier than Tox. Totally looks doable.
That deploy
section in the Travis file - is that provided by setuptools_scm
?
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.
Oh wow nvm this is just a TravisCI feature. Well that's freaking cool
setup.py
Outdated
'Topic :: Software Development :: Libraries :: Python Modules', | ||
] | ||
|
||
version = environ.get('RELEASE') or environ.get('VERSION') or '0.0.1' |
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.
Shouldn't the package have a __version__
attribute to read here? 🤔
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.
Got me dude, I made this all up. I put the version in the Travis file. Where should I be putting it?
setup.py
Outdated
packages=find_packages(exclude=('tests')), | ||
cmdclass={'test': Tests}, | ||
tests_require=[ | ||
'mock', |
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 should only be required in Python 2... in case it were used, but it isn't.
# -*- coding: utf-8 -*- | ||
# Copyright 2016 LasLabs Inc. | ||
# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). | ||
|
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.
@lasley This file must only contain __path__ = __import__('pkgutil').extend_path(__path__, __name__)
to keep oca as a reusable namespace package.
https://packaging.python.org/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages
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 think the complexity of having the package split doesn't pay. It's just easier to have all in one place, this shouldn't grow up so much as to be unmaintainable. Just a collection of helpers...
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.
@yajo I think we must keep the possibility to release others python packages under the oca namespace. (ie: utilities to use in our dev/release process, ????). There is nothing complex here.
setup.py
Outdated
'Programming Language :: Python :: 3.4', | ||
'Programming Language :: Python :: 3.5', | ||
'Programming Language :: Python :: 3.6', | ||
'Topic :: Software Development :: Libraries :: Python Modules', |
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.
@lasley You can also add Framework :: Odoo
into the list. It's now an official classifier https://pypi.python.org/pypi?%3Aaction=list_classifiers
from .foreach import foreach | ||
|
||
|
||
__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.
@lasley You can leave this file empty.
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 is required in order to make the foreach
method inside of the foreach
file directly importable.
Otherwise usage is:
from oca.decorators.foreach import foreach
instead of
from oca.decorators import foreach
Do you have a better way to solve this?
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.
👍
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.
Do we really want one file per decorator? 😲
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 generally prefer deeper file hierarchies with smaller files. I've found that if I don't do this type of thing from the get-go I end up in a situation where the files are thousands of lines deep, I spend a bunch of time trying to categorize things, then just lose stuff anyways.
I'm open to consideration here though. You would prefer to categorize/group somehow?
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.
@lasley IMO Your way of organizing your code is safe.
setup.py
Outdated
pass | ||
|
||
|
||
class Tests(Command): |
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.
What's the purpose of this class?
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.
It discovers tests to run, then runs them. I prefer to have the isolated class, because our internal CI can inject XmlTestRunner
for parse by JUnit
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.
@lasley IMO we must keep setup.py the must simple as possible and only put into this file the information required to package and setup the code. Moreover, we can simplify the content of .travis.yml
to only use standard python tools. I'll propose you a PR.
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.
requirements.txt
Outdated
@@ -0,0 +1 @@ | |||
decorator==4.0.10 |
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 this is a library, do not pin version.
setup.py
Outdated
setup_requires=[ | ||
'setuptools_scm', | ||
], | ||
install_requires=install_requires, |
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.
So install_requires=['decorator>=4']
should be sufficient.
setup.py
Outdated
|
||
|
||
install_requires = [] | ||
if path.exists('requirements.txt'): |
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.
@lasley you should remove the requirements.txt file and declare you dependencies into install_requires
setup.py
Outdated
setup_requires=[ | ||
'setuptools_scm', | ||
], | ||
install_requires=install_requires, |
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.
@lasley install_requires=['decorator']
If you need a minimal version: install_requires=['decorator>=4.0']
Oops sorry I forgot about this one. Requirements.txt removed, decorator lib unfrozen (I don't know a specific version so it's blank), and added to |
* Travis * Coveragerc * Gitignore * License * ReadMe * requirements, include Odoo * setup [ADD] Foreach API (#1) * [ADD] Foreach API * Add foreach API * Change license to LGPL (odoo dependency) * Add return value assertion to foreach api * Fix docblocks and make thing super efficient! * Dumb it down! * Fix docblock to have call * Rename api to helpers [FIX] CodeClimate Badge [FIX] Update pip instructions, fixes OCA#3 [IMP] Update Travis, Setup, ReadMe * Add PyPi to Travis * Add version definitions to setup * Add current badge structure to ReadMe [IMP] Prep for OCA release * Add Python 3 support * Change author from LasLabs to OCA * Update repos * Move `helpers` package to `decorators` Switch versioning, license Namespace/package updates, add Odoo framework Fix docblock for Tests Fix travis Revert decorators init
* Simplify tests * Add codeclimate * Support Py36 * Remove Py34 from classifiers into setup.py
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.
@lasley LGTM. (Code Review Only)
Some of the pip pros please hit the button 😊 |
Now time to see if this actually pushes to PyPi. I need to make a tag yeah? |
@lasley yes |
Damn it seems I did the Travis file slightly wrong. I made the fix on master and retagged. We're now on PyPi!!! https://pypi.python.org/pypi?:action=display&name=oca-decorators&version=0.0.1 |
Hmmm but what about documentation? My build system did that as well, but it's blank and I don't see Travis attempting anything. |
@lasley IMO You must modify .travis.yml to gen and publish the documentation. You should find plenty examples on the net on how to do it... |
But I had documentation until Tox was added. I have never been able to get Travis to automatically compile my Sphinx docs using anything other than the build system that I created, which was removed in favor of a strategy I advised I knew nothing about and was not able to get to work. |
@lasley I'll experiment a simple way to do it on Alfodoo and come back with a PR on oca-decorators once it's done. I plan to use https://github.com/Syntaf/travis-sphinx/ |
This is one of those times I wish I kept better notes, I remember trying Edit: Fixed the duplicated link & added sphinx config link |
If I remember correctly, OCA/connector has a decent way to generate doc. |
This is the initial release of the OCA-decorators package. It currently contains the
foreach
API.