New checker for JavaScript language. #10
Conversation
|
I am pretty sure that tests are failing on Travis due to lack of |
|
If you think that system dependency is missing you can extend .travis.yml with proper before_install section. |
|
Ok, so I did the clean run ( |
|
https://travis-ci.org/klen/pylama/jobs/11958164#L26 python-gflags is installed correctly: $ sudo apt-get install -qq python-gflags
Selecting previously unselected package python-gflags.
(Reading database ... 62342 files and directories currently installed.)
Unpacking python-gflags (from .../python-gflags_1.5.1-1build1_all.deb) ...
Processing triggers for man-db ...
Setting up python-gflags (1.5.1-1build1) ...however I don't really know what are other differences between your env and travis... |
|
|
|
on Travis: |
no, but you can say 'yes'... $ add-apt-repository --help
Options:
-y, --yes Assume yes to all queriesBut I've looked into the problem now... and don't get why you need system packaged instead of python egg python setup.py develop[test]during travis build, so it would install all deps needed during test... |
|
So I can reproduce and fix (even with |
|
I do not need sys-package, it can be python egg too. I just have read that this way you are "more sure" things will work for all Linux distr. - it worked locally for me, it works on a different machine with different Ubuntu right now, as you can see the log above, so I decided to take this "recommended way". Your suggestion is to extend right ? or, not? Sorry, for so many questions buy this is my "first time" with Travis and I did not expect such issues. |
|
I thought that project uses setuptools setup.install_requires so you could later: $ python setup.py develop # for regular dependencies pyflakes pep8 pylint
$ # and
$ python setup.py develop[test] # for test dependenciesbut we have to explicitly call 'pip install -r ' instead. It's a bit of an inconsequence of library's author. Using egg has this advantage over system requirement that is system independent, so we won't need to inform travis for installing different things on different systems.
I have never used Travis too, no need for sorry :) |
|
Ok so my idea was to make it "light way", the core purpose of PYLAMA is to check Python code, so lets say 80% users will use this for Python (so why do they have to install packages required by Did I get it clear that, you suggest to integrate install_requires = ['python-gflags']
if version_info < (2, 7):
install_requires += ['argparse', 'ordereddict']? |
|
update: |
|
Ok, please rebase it, so it contains only one commit with meaningful message. |
… into gjslint_checker
|
All modifications (commits) done on this pull request (driven by Travis issues) are squashed into |
Are you sure that all commits are squashed into one commit so this pull request contains only one commit? |
|
I understood "so it contains only one commit with meaningful message" as you want to have
Please review the source code. |
|
Hi, I'll be your reviewer for this pull request. Sorry for my english, just ask if something is not clear.
One commit is better for merger/upstream because it's easier to maintain/revert later. "Showing effort" isn't necessary, as the end result of your work is much more important for everybody. "commit often" practice is good when you're working on changes locally, but overall pushed result should be contained in one commit with commit message in format used by project. It's more infomation/guideline how we should work. |
|
Hi Jarek :) I see your point but I just disagree. Please let me try to exlplain why:
But if you need me to follow some rules I will try to do my best. |
| class LamaJsTest(unittest.TestCase): | ||
|
|
||
| def test_gjslint(self): | ||
| if version_info < (3, 0): |
There was a problem hiding this comment.
I think we can use unittest.skipIf. It's more explicit than if inside test body.
There was a problem hiding this comment.
Agree if we think about the same: @unittest.skipIf(sys.version_info < (3, 0), "not supported in this veresion"), you are right. But when you take a look on the code of Pylama author, there you can find already existing convention for skipping the tests for not supported Python versions. I just followed already existing convention. But sure thing I agree to what you've pointed out.
There was a problem hiding this comment.
Could you please be more verbose? I just found link to package. Or please discuss my proposition. However I still think participating in shared code should not change the convention of the author. Maybe switch to "new skipping style" can be introduced to the author for all cases as a separate PR - what do you think ?
There was a problem hiding this comment.
I mean - unittest2 is backport of 2.7's unittest which you can use to implement this right way.
I think it's really important to diverse state of test between skipped/executed but without insides.
|
I've described a way in which i prefer to work. I don't want to cause too much discussion which isn't point of this PR :) |
|
Sorry Jarek for following this topic in this PR but I think it is really important. Maybe you can take a look on this: after Rober Conckling: The problem with rebase is that it corrupts the true history of the commits. This doesn't show up on a smaller repo, but if you have a busy repo, with lots of contributers, untangling a mess becomes much harder if you no longer have the true parentage of a given commit. Git rebase destroys the context of the commit, leaving basically a diff apply instead of the much more contextually rich merge commit. Yes, your repo looks messier, but it more accurately reflects the lifecycle of the code, and what the developer intended at each commit. If you really want a straight-line for your repos, why aren't you using something like SVN? Is the distributed nature of git really the big selling factor? I've run teams that have used git how you are describing, always making people use rebase (in this case it was because we were syncing to SVN). Reconstructing what a developer did 4 months ago is much simpler with a merge vs a rebase. You can go to the merge just before the developer's first commit and see what the repo actually looked like when they started work. You can never do that with a rebase. The context in which the developer was working has been lost. Also, with multi-commit branches, you can see what the repo looked like after each commit. With a rebase, those intermittent commits are almost useless, as the repo doesn't look like it did when they made that commit. To sum up, don't subvert git just because you want to see a straight line in your commit graph. It's not worth it, and it is ultimately a lie. I am just curious what do you think - cuz we are both using git right ? So why not to lear sth. new form each other ? |
|
Guys, this is getting out of hand. I understand Lukasz's point, but think that it is mainly influenced by the job that he's currently doing. Right - rich log is critical to reason about the large code repository, which is the main goal of the tool that Lukasz is developing right now. However, I think that something need to be highlighted here. What we tend to do here is to create small and atomic issues, so as they fit into one commit. If you need to split it into more meaningful and atomic commits - split the issue accordingly. What is more, I prefer code to speak for itself, not relying on commit messages. Code itself should be clear and if an important assumption needs to be made that is far from obvious - it's a perfect use case for "in code comments", not commit messages. A commit of: "extend setup.py and clear travis.yml" is of limited use to anybody - you missed something, which was a part of the issue that you try to resolve - that's all this commit is for. We extensively use issue trackers / wiki pages / sphinx doc pages to keep track of the history of the project not the git log. I don't want to fight here - simply different approaches to the task. Let's agree to disagree here and don't follow up on the topic anymore. Changes made to the code to solve the task assigned are clear - you can leave them as they currently are - no rebase needed. |
|
@but think that it is mainly influenced by the job that he's currently doing - I think you got me here ;p I am too much influenced by my way. Srx for trying to force my thing on top. From now on I will stick to the rules w/o discussion. |
| @@ -0,0 +1,2280 @@ | |||
| /* =================================================== | |||
There was a problem hiding this comment.
Why this file is in main directory? It looks like bootstrap.js library. Could you explain purpose of this file?
There was a problem hiding this comment.
You have dummy.py file used for tests for Python checkers in the main directory - so following proposed by author convention you have dummy.js files used by JavaScript tests in main directory. Why bootstrap - in general random pick, but it is open source, popular and I found it to be a good test case for gjslinter.
There was a problem hiding this comment.
I think you can replace by much smaller js file as we don't want to test internals of gjslint. Just upload file with hello world and comment about this file purpose. Also i would move this kind of files into tests/data as i think they shouldn't pollute global codespace.
|
Ok, I think we can go into next round of review :)
|
|
Sorry for long time I've been very busy at last weeks. And thank you for work. So, I saw the request and made some decisions. I will not include gjslint to pylama module by default. Not so many people are required check js files with pylama. BUT, I created some like plugin system. In the basic version pylama will be only consist 'pep8,pyflakes,mccabe,pep257' checkers. By example, for pylint support you should install 'pylama_pylint' package. So, I suggest you move gjslint to "pylama_gjslint" package and will be maintainer for them. I could do it myself, but it's your plugin. It's very easy, see realization in pylint: https://github.com/klen/pylama_pylint and look in Pylama's README. If you have any problems, questions or etc, I would like to help. |
Scope of this pull request:
def gjslintcallback inpylama\utils.pyto handlegjslintlinter request,main.pyby.jssuffix - default.pyis always present due to main purpose of the tool,tests.pycovering added functionalities (to test JavaScript codedummy.jswas added to the root of Pylama)READMEbygjslintentries,requirementssection inREADMEbypython-gflagsandcursesfor Win platform users (after issues when running on clean Python 2.7 virtualenv).Attention:
gjslintdue to issues like: usage ofStringIOinclosure_linter/tesutil.pyetc.Requirements:
python-gflagssudo apt-get install python-gflagsand verify installation using:dpkg -s python-gflagspip install python-gflagsand verify installation using:pip freezeTests:
python setup.py installended successfully.Notes:
Printing utilities were removed due to common printing interface provided by Pylama.
Usage:
pylama.exe --linters=gjslint --ignore=E:0010 --report=report.txt E:\path\to\dir_or_filethere is just JavaScript code or just Python code, then just this linter will be used
for which the code was found. Example:
If in
E:\bootstrap\jsthere are just*.jsfiles. Usage:pylama.exe --linters=pep8,gjslinter E:\bootstrap\jswill use justgjslinter. It works exactly the same the other way round.