[IMP] Making package #500
[IMP] Making package #500
Conversation
|
@nhomar IMO this package should be named oca-mqt and mqt put under the oca namespace. |
| Or, if you have virtualenvwrapper installed:: | ||
|
|
||
| $ mkvirtualenv mqt | ||
| $ pip install mqt |
There was a problem hiding this comment.
@nhomar pipsi is a nice libl to install python script (https://pypi.python.org/pypi/pipsi) Python script installed with pipsi are available as command line and moreover are transparently installed in it's own virtualenv.
There was a problem hiding this comment.
I think we should mention that as a suggestion but ppl can install this sys wide too.
There was a problem hiding this comment.
@simahawk I's true that pip can install this sys wide, unfortunately if you do it, you 'pollute' your python sys env. Therefore IMO it is not the recommended way. It would be nice to mention that a lib can be used to circumvent this problem.
There was a problem hiding this comment.
@lmignon I expressed myself poorly. I meant "suggest pipsi" and recommend venv over sys wide 😉
There was a problem hiding this comment.
I will share how it works on this case, I will read about pipsi @lmignon can you point me to a link/video with the PoC of what you want to achieve with such recomendation?
There was a problem hiding this comment.
Ya got it! forget the last message I will recommend that in the doc.!
Just FYI I install with pip install --editable . inside a virtualenv for development purposes. LEt's see if that is possible.,
There was a problem hiding this comment.
There was a problem hiding this comment.
thanks @lmignon, I will follow your lead that in the doc itself.
| @@ -0,0 +1,83 @@ | |||
| import sys | |||
There was a problem hiding this comment.
@nhomar Why to you need this compat layer? It seems that a lot of alias defined in this file are not used.
There was a problem hiding this comment.
copy paste from the cookiecuter template, I am on WiP cleaning up... can you kindly check the doc of the cookie cutter template mentioned in the description of the PR please? @lmignon
There was a problem hiding this comment.
But if we son't need it we should remove it and only add/keep what' relevant for this lib. But IMO we can be PY2/3 compatible without this compat layer.
There was a problem hiding this comment.
+1 @lmignon I will remove that.
| click.echo('Running OCA Maintainer quality tools.!') | ||
|
|
||
|
|
||
| @cli.command() |
There was a problem hiding this comment.
@nhomar IMO each command should be defined in the module where it is implemented and added as subcommand of a main command.
In short:
- into setup.py you define one entry point to create a console script for your main command
entry_points='''
[console_scripts]
oca-mqt=oca.mqt.cli:main
'''- into cli.py you define the main method with the common args to all the sub commands. You can store the value of common option into the 'ctx' objet. This object is passed to all the sub commands.
@click.group()
@click.version_option(version=__version__, message=__notice__)
@click.option('-v', '--verbose', count=True)
@click.option('-c', '--config', type=click.Path(dir_okay=False, exists=True),
help="Configuration file (default: ./mqt.cfg).")
@click.pass_context
def main(ctx, verbose, config):
ctx.obj = config
if verbose > 1:
level = logging.DEBUG
elif verbose > 0:
level = logging.INFO
else:
level = logging.WARNING
logger = logging.getLogger()
channel = logging.StreamHandler()
channel.setFormatter(ColoredFormatter())
logger.setLevel(level)
logger.addHandler(channel)- into each module providing sub command, you declare the sub command and add it to the main command.
lint.py
from .cli import main
@click.option('paths', '--path',
envvar='BUILD_DIR',
multiple=True,
type=CLICK_DIR,
required=True,
default=[os.getcwd()],
help="Addons paths to check pylint")
@click.option('--sys-paths', '-sys-path',
envvar='PYTHONPATH',
multiple=True,
type=CLICK_DIR,
help="Additional paths to append in sys path.")
@click.option('--extra-params', '-extra-param',
multiple=True,
help="Extra pylint params to append in pylint command")
@click.option('--msgs-no-count', '-msgs-no-count',
multiple=True,
help="List of messages that will not add to the failure count.")
@click.pass_context
def lint(paths, ctx, msgs_no_count=None,
sys_paths=None, extra_params=None):
"""Test the pylint an odoo-addons folder."""
try:
fname = cfg.config.name if cfg.config else False
stats = run_pylint(
list(paths), cfg=fname,
sys_paths=sys_paths,
extra_params=extra_params
)
count_fails = get_count_fails(stats, list(msgs_no_count))
except UserWarning:
count_fails = -1
click.echo(count_fails)
main.add_command(lint)In this way you improve the modularity, the readability and the maintainability.
To run the subcommand lint you just need to type:
oca-mqt lint
There was a problem hiding this comment.
I personally prefer only one file with all the cli parts (that little piece of code is yet candidate to be removed).
My statement is that the CLI file for me is like the layer view in a MVC architecture.
Then all my cli should not have code with logic, just calling method with parameters, into the index of commands in only 1 place, that point to different Agnostic Python modules.
I think it is easiest to document and to maintain as an unique package.
I do not know if that make sense for you?.
@lmignon (let's discuss what I say and not what I do, remember I am just copying pasting the current code, trying to not refactor anything to move step by step).
About Modularity and plugins I think it is another story.
There was a problem hiding this comment.
@lmignon Another point about the proposal of architecture I am using is ensure that everything can be importable in a separate script for custom usage (not considered here).
i.e:
I make my own script without patch mqt itsef that will make:
from mqt import some_tool
import click
@click.command
def custom_stuff():
do_your_own_magic()
some_tool(**params)
Do that make sense for you?
There was a problem hiding this comment.
@nhomar The problem with this approach is that your 'cli ' must be aware off all the command (a kind of one2many). IMO it's the responsability of a sub commad to register itself into the command registry. In your approach the command registry is responsible to register all the command.
yours
cli.py
from . import x
from . import y
from . import z
def cmd_x():
def cmd_y():
def cmd_z():
If you want to add a new command you must modify cli.py and add the module providing the logic
my proposal
cli.py or main.py
def main()
x.py
from .main import main
def MyClassX():
...
def my_method_x1():
def cmd_x();
main.add(cmd_x)
x is a module that can be used where you want and x register its command into the command registry.
If you want to add a new command you just have to 'import main ; main.add()' from where you want. (from other module into the oca-mqt package or even from your internal package that leverage oca-mqt for your specific needs)
There was a problem hiding this comment.
@nhomar The problem with this approach is that your 'cli ' must be aware off all the command (a kind of one2many). IMO it's the responsability of a sub commad to register itself into the command registry. In your approach the command registry is responsible to register all the command.
@lmignon Thanks for the proper explanation, I think I perfectly understand, well! what you see as an advantage I see it as a problem then we have different PoV there, can I move on this direction as I am doing it now to measure properly the results of my PoV and then if not, it will represent just move 4 lines methods to their proper files which can be done in 5 minutes, Agree?.
It is not necessary to expend a lot of time there now, because neither you and I am wrong on my option opinion, On my option we will see only one isolated list of method with TONS of documentation for such option on the command line itself because the point is have a very clean file very well documented in the command line. With yours it will be (in my opinionated PoV) a mess to control that something is properly documented in the cli sub commands.
Then as technically it will not represent a huge problem once we finish the first version we can change it if really necessary. Agree?
mqt/lints.py
Outdated
| return pylint_res.linter.stats | ||
|
|
||
|
|
||
| def flake(): |
There was a problem hiding this comment.
@nhomar IMO it would be better to split each sub command in its own module.
pylint -> pylint.py or ???.py
flake8 -> flake8.py or ???.py
There was a problem hiding this comment.
Agree. Maybe:
linters/common.py (helper methods here)
linters/pylint.py
linters/flake8.py
There was a problem hiding this comment.
flake is so so simple (just 10 lines) it was separated before and I just join them because the name of the file is "lints".
I do not see too much value have 2 files for that and a sub folder.
Remember that ones something will become big enough then may be extract from here will be proposed.
but I will considere this.
There was a problem hiding this comment.
@nhomar I prefer to keep thing separate from the start. Once you split a module, you lost the
change histrory or at least it become more difficul to follow it. Moreover it's also to stay consistent with what we ask for odoo addons. One module by model. (it's just my POV).
mqt/travis_helpers.py
Outdated
|
|
||
| def colorized(text, color): | ||
| return '\n'.join( | ||
| map(lambda line: color + line + CLEAR, text.split('\n'))) |
There was a problem hiding this comment.
return '\n'.join([color + line + CLEAR for line in text.splitlines()])
There was a problem hiding this comment.
Remember it is a copy-paste of original code. considering.
mqt/mqt.py
Outdated
| ) | ||
|
|
||
|
|
||
| class Mqt(object): |
There was a problem hiding this comment.
is this the placeholder addressing "NOTHING can be moved in the meanwhile we must maintain the scripts as they are AND create new files to make them Importable Classes to be used in the click CLI interface."?
Meaning that will have a Linter class for instance?
There was a problem hiding this comment.
@simahawk sorry for the last unexplained answer, Yes, that's what I am trying I will use that class for the creation of the environmental decisions in order to have such place already set to avoid design mistakes in the move of the other scripts.
There was a problem hiding this comment.
Meaning that will have a Linter class for instance?
@simahawk Can see on the current status what that class will be used for, that make sense for you?
Can you enlight me with some example which technical value add such? I think this package is a set of tools that are only and refered to as mqt then why migrate/change/add over namespaces if this package will not do anything else more that what it was designed for "Tools to maintain the ......." I listen but kindly can you help me with the technical value? or your vision to align mine? |
|
Hello People. I am trying to resolve the migration stuff and in order to avoid deliver the impression of something deliberately incorrectly set I will try to clean up the first script pretty close to how I think they will stay at the end (again with no touch the original ones). I will try to list the FEATURES that we actually do in order to test them on your side once the first PR demo is merged. |
https://github.com/OCA/oca-decorators is structured in this way. I can help to make the required changed. |
|
@nhomar When you say you want to keep the backward compatibility can you elaborate? IMO the compatibility should be achieved by a compatibility layer. In the case of MQT, the best candidate for this compatibility layer is the existing entry points (IOW all the shell scripts). |
@lmignon For now mqt is not being used as installable scripts themselves but retreived directly from the sources in every build. We have 3 options here:
Problems @lmignon I edited the message because I confused the numerical part kindly read on github.
I decided use 1st in a dictatorial manner even if (I am agree with you) we should have a compatibility layer, but this package specifically is not prepared for such move, then in a second step (maybe 2 or 3 weeks) use the 2nd approach, to have something cleanly prepared to work with the 3rd approach for now on (because we will have all prepared). I mean, you are right but now is not the moment to think on such layers, as I said before I do not want:
I hope it explain better my rationale about the way I want to move the project, can you help me even if we move on that direction?. |
|
About the namespaces: Even if I love promote the OCA name we can not put marketing stuff to make technical reasons, that complicate the development itself. Some of the most important package in the Python world are managed They manage:
And they DO NOT need to say and complicate the development to have a complex name space management stuff saying:
What will become memorable our packages will be the quality of them and the easiest way to contribute to, and their documentation, that will become them the de facto way to work with odoo from the private and the public perspective. Let's no complicate developments for non-technical reasons please. We have another community that manage a huge number of packages on the python world like: May be you even do not know they did them, the nice part is that they have REALLY big people documentation policies we should point on that direction, and see how they manage their documentation like a charm:
AMAZING isn't it? Let's not make our technical decision be based on marketing plans that do not add any value. Can We? Imagine once we achieve that size maintain a compatible common namespace? that will be insane ;-), every team will have their own rules for their packages and so on, but this package witll be to centralize the SQA layer then lets maintain it on that way (openstack has such set of tools and sub-tools for that also) and that's the plan here:
I hope it explains my PoV. Agree? BTW I just registered temporarelly the mqt name on pypi. PS: Under my user (we need to define an OCA user, I did not know if we have one) to distribute it in PyPI into the plan no argument I WILL MOVE under the OCA user it was just to get the mqt name on pypi. |
|
bottom line comment about the name space: IF you want to move it to another name rather than |
|
@lmignon @nhomar I suggest to: let Nhomar finish the porting 1 to 1 of current situation and merge it into |
|
@simahawk It's ok for me. I'm not ready to live into a dictatorship 😏 |
thank U ;-) |
place where document, then, this change does that, only the minimal structure to have all the documentation auto-generated and auto-tested [IMP] Adding files to work as a complete documented package. - Added minimal cli structure (didactical commit). - activating tox [FIX] adding run-test to standarize the test environment. Add missing file to manifest [IMP] command `mqt lint` command is working as a shell command. [IMP] Remove all reference to name TRAVIS in filenames and variables. - All what will be related to travis specific is the one that should be called **travis** if not then use agnostic-brand names. m [IMP] mqt lint: Taking a defailt cfg in order to save lint config file declaration when not needed. [IMP] mqt flake8 running (yet things to do) [REF] s/travis_helpers/helpers/g no reference to helpers. m [IMP] more cleanup doc is compiling properlly with the new approach. [IMP] Several fixes in the doc and naming yet wip [DEL] cleaned file already moved to lints.py bumpversion Bump version: 0.1.0 → 0.1.1 WIP: Converting Pylint process in a class m tmp
nhomar
force-pushed
the
master-making-packge
branch
from
d00f65e
to
6e0373f
Compare
| @@ -0,0 +1,133 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Could just do as https://github.com/OCA/maintainer-quality-tools/blob/master/git/getaddons.py?
This way the file is unique.
And also do it for the other copied files.
|
@nhomar any news on this or it can be closed? |
Rationale:
Using the knowledge behind this cookie cutter template (in order to avoid empty discussions about some technical decisions) starting with the very first steps to have a generic python Package.
TODO:
mqt --parameter dothen document the roadmap about that.import git_agregator; git_aggregator.do_magic()and update the repositories, this can not be done in 1 step).Once discussion is open this TODO can grows up.
We expect start with this work and be able to Having the structure start to move element by element, well to inside a proper place or to external tools (python libraries) that can be Importables and used here, we need to be careful to not try to convert ALL in an extra python package, but neither left here things that can be generic enough to be used stand alone that must be discussed case by case.
Excellent presentation to watch in order to make some decisions.