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

[REF] pylint script: Migrate bash pylint script to python. #229

Merged
merged 2 commits into from Aug 20, 2015

Conversation

moylop260
Copy link
Contributor

No description provided.

:return: Integer with quantity of fails found.
"""
count = 0
for msg in linter_stats['by_msg']:
Copy link
Member

Choose a reason for hiding this comment

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

All messages are errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, here return all checks activated in cfg and found.
If a check is returned then is a fail. (Currently bash script is working equal)
If you dont want a fail then you remove the check from cfg file

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification

@pedrobaeza
Copy link
Member

OK then

You have my 👍

@moylop260
Copy link
Contributor Author

@pedrobaeza
Thank for your comments.

@moylop260
Copy link
Contributor Author

FYI travis-build using this PR without pylint fails and other one travis-build-w/fails

@pedrobaeza
Copy link
Member

@dreispt, @yvaucher, @ALL, can you review this as it's easier to review to move on with each point?

@yvaucher
Copy link
Member

👍 just discovered the click pylib

Thanks @moylop260

@nhomar
Copy link
Member

nhomar commented Aug 19, 2015

👍

@moylop260
Copy link
Contributor Author

Give me a second please, before of merge.
I'm testing a case...

@moylop260
Copy link
Contributor Author

By the good comment of @pedrobaeza
I did consider adding the test to see if he was right.
The second commit have this case considered and fixed.
My apologies for not anticipate at the time.

👍

NOTE: Can live with 2 commits (no rebase) for don't have that change the other progressive PR (just a git pull)?

@moylop260
Copy link
Contributor Author

I merged this PR in our forked project and work fine!
What is the next step we will wait another plus one or we can merge it?

@pedrobaeza
Copy link
Member

We have 3 approvals, so merging to continue with next steps

pedrobaeza added a commit that referenced this pull request Aug 20, 2015
 [REF] pylint script: Migrate bash pylint script to python.
@pedrobaeza pedrobaeza merged commit b558350 into OCA:master Aug 20, 2015
@lepistone
Copy link
Member

Hi all, I have a failing build here:

https://travis-ci.org/OCA/runbot-addons/jobs/76416680#L241

Unless anyone sees the problem real quick, what do you think about reverting this one? Thanks!

@moylop260
Copy link
Contributor Author

@lepistone
I will to run a travis2docker to detect this problem.
Can you give 25 minutes, please?

@lepistone
Copy link
Member

thanks a lot @moylop260

@moylop260
Copy link
Contributor Author

@lepistone
I could reproduce the error into a docker with travis2docker.
The problem is that the folder:
ls ${TRAVIS_BUILD_DIR}
is empty
We don't have to consider it this case in our script.

I will to fix it to exit(0) and print a msg when the path is empty.

Now, with or without this fix... in your builds the lint's never are checked.
Is this a expected feature?

Maybe you could add an if before to rm all modules if is a lint check here:
rm -rf $(ls | grep -v runbot$)
https://github.com/OCA/runbot-addons/blob/8.0/.travis.yml#L36

@lepistone
Copy link
Member

Thanks for your answer @moylop260. I'm lost here: I don't see why should the rm directive be relevant: it deals with a dependency (odoo-extra) that should not be under test. We are testing runbot-addons.

Also can you explain about TRAVIS_BUILD_DIR? I don't get that neither.

@moylop260
Copy link
Contributor Author

Ok, I got it
The command rm -rf $(ls | grep -v runbot$) is executed into default folder ${TRAVIS_BUILD_DIR}
You need to change the command with the full path:
rm -rf $(ls ~/odoo-extra | grep -v runbot$)

@moylop260
Copy link
Contributor Author

@moylop260
Copy link
Contributor Author

I have created a new video with your current case video

@lepistone
Copy link
Member

Thanks @moylop260! OCA/runbot-addons#53

@moylop260
Copy link
Contributor Author

I will to add this case in our selftest of MQT and fix it to catch this problem.

@guewen
Copy link
Member

guewen commented Aug 21, 2015

I disagree with the addition of the click library here.
As neat as this lib can be, I think we shouldn't use it in MQT for the following reasons:

  • the code which runs the tests should be as simple as possible, adding a new dependency such as click increases the chances of bugs
  • MQT must be installed on every build, so it should be as fast as possible. Adding a new dependency should be considered with care
  • Click (a CLI library) is useless on Travis, there is no human on Travis to use the CLI
  • Ultimately, if you really want a CLI because you clone the MQT on your project, there is nothing here that can't be done with the standard lib.

@moylop260
Copy link
Contributor Author

I can work to remove click after of finish all my pending pylint pr without problem.
I'm agree with you.
I just used because I saw guidelines and I dont found nothing that deny it.
I appreciate if you add your cool rules in our guidelines.

@moylop260
Copy link
Contributor Author

And Can you add in guidelines our "standard lib" list too, please?

@guewen
Copy link
Member

guewen commented Aug 21, 2015

Which guidelines do you talk about?

@nhomar
Copy link
Member

nhomar commented Aug 21, 2015

2015-08-21 8:32 GMT-05:00 Guewen Baconnier notifications@github.com:

Which guidelines do you talk about?

Guidelines that explicit say "What is considered 'Use a new lib' and what
is considered 'Use a python lib'?

If the feature "USe click" is considered a feature, we should have a
guideline that explicitally mention that we should make a separate PR.

For use use click is part of "use the tools"!


Saludos Cordiales

CEO at Vauxoo https://www.vauxoo.com Odoo's Gold Partner.

[image: --]
Nhomar Hernandez
[image: http://]about.me/nhomar
http://about.me/nhomar?promo=email_sig

@guewen
Copy link
Member

guewen commented Aug 22, 2015

@nhomar We shall not have a guideline forbidding the usage of external libraries. Taking the decision to add a new library should be motivated by good reasons and it is not the matter of rules or guidelines but is exactly the matter of code reviews. Against click in the MQT, my arguments are explained here #229 (comment) and I am disposed to expand on them if required.

@moylop260 The argument parsers of the Python Standard Library are optparse and argparse.

@yvaucher
Copy link
Member

Now, I tend to agree with @guewen MQT should be light and super fast, it is intended to be launched only by travis and several times by hours. What we could do is making other repos to be used by humans implementing a more user friendly way of launching lints and tests which could be less lightweight based on MQT files.

@pedrobaeza
Copy link
Member

But with these changes you still have a super-fast suite, but more understandable and hackable (certain blocks of code are not executed if not activated with parameters). I don't really understand your reluctancy...

This PR is merged already 4 days ago, and you can check new Travis checks. You won't find any difference in times before and after this change.

@nhomar
Copy link
Member

nhomar commented Aug 24, 2015

On this point about the "fast suite" everybody is agreed, no question about that, and that is a topic which is considered also here.

In futures points we can tend to discuss about "specific" topics, because if we continue talking generally speaking without specific topics then simply we are wasting lot of time.

Are you agreed to discuss this on the proper mailing list?.

@nhomar nhomar deleted the oca-pylint-bash2py-moy2 branch August 24, 2015 09:34
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.

None yet

6 participants