Summary

[asmeurer]

This pull request refactors SymPy-Bot a little bit.

Two chief things were done here (unfortunately it would have been too much work to split them into separate commits):

1. SymPy-Bot now avoids doing redundant things more than once. In particular, for each pull request, the merge is attempted only once, and use2to3 is run only once. Note that py3k-sympy is not cleared between pull requests either, which might potentially create false positive test failures, at least until SymPy issue 3344 is fixed. Also, the branch that is tested is now called test_N, where N is the number of the pull request (like test_123). The branch is not deleted when the tests are done, so if you want, you can checkout the temporary directory and test things.
2. The review summaries have been greatly condensed. Redundant information (i.e., branch hashes) is shown only once at the top, in a clearer English format that now gives the branch names and was inspired by Travis-Bot. Default information, such as the test command, is only shown if it differs from the default. Finally, all url links are now placed inline with the text. The result is a comment summary that I believe not only takes up significantly less space on the pull request page, but is also easier to read.

Finally, as one additional note, SymPy-Bot now officially only runs in Python 2.7. This was so that I could use Python 2.7 idioms like set literals and dictionary comprehensions that make the code easier to read (and Python 2.6 support was dubious anyway).

Please, please, please test this before merging it. As I noted, it refactors quite a bit of the core logic. I've tried to test all possible code paths, but I'm sure I've missed something.

[asmeurer]

Ah, apparently what I wrote about only running use2to3 once was wrong. I Intended to do this, but I guess I didn't get around to it yet.

[asmeurer]

Here's a before and after.

[Krastanov]

This will break the script that I am using for the bot. However the bot that I run is already of little use as we have travis and your new machine with the "test-all" profile. The only purpose that it is still serving is pointing out the hash related errors. Thus I will not update the script for the new comment format (it was a throw-away script anyway).

Anyhow, I am in favor of the changes here, however I have only skimmed through the code.

[asmeurer]

How will it break the script? The change in the hash format?

[asmeurer]

And my test-all profile is not a replacement for your script. What I would need is something that runs the bot automatically.

[Krastanov]

The script is searching with a regex for "branch: hash" or something like that. It can be fixed, however I am unsure whether it is worth it. If someone finds it useful I will update it of course (it should be a one-line change)

[Krastanov]

Obviously, a better solution is to actually use the github api instead of my hackish code, however I won't have the time to do this anytime soon.

[asmeurer]

Is your script somewhere?

[Krastanov]

Is your script somewhere? You can find it here https://gist.github.com/2985162

[certik]

Obviously I am +1 to the changes. We just need to test it a bit.

[certik]

So I merged your branch with master on my computer. Here is one bug:

$ sympy-bot list
Traceback (most recent call last):
  File "/home/ondrej/usr/bin/sympy-bot", line 524, in <module>
    main()
  File "/home/ondrej/usr/bin/sympy-bot", line 150, in main
    val = getattr(options, i)
AttributeError: 'Namespace' object has no attribute 'build_docs'

[certik]

Another problem. I want to review using Python 3.x, so I did:

$ sympy-bot review -3 1407
usage: sympy-bot review [-h] [--profile PROFILE] [-n] [-2 [PYTHON2]]
                        [-3 [PYTHON3]] [-D [BUILD_DOCS]] [--no-comment]
                        [-i INTERPRETER] [--interpreter3 INTERPRETER3]
                        [-t COMMAND] [--build-docs-command COMMAND]
                        [--build-docs-dir DIR] [-r REFERENCE] [-m COMMIT]
                        [-p {https,git}] [-s SERVER] [-R REPOSITORY]
                        n [n ...]

Reviews specified pull requests.

positional arguments:
  n                     Numbers of pull requests to review. You can also
                        specify 'all' or 'mergable' pull requests.

optional arguments:
  -h, --help            show this help message and exit
  --profile PROFILE     Configuration file profile to use, see README for
                        information about setting up profiles (default:
                        default)
  -n, --no-upload       Do not upload the review to the server (default:
                        False)
  -2 [PYTHON2], --python2 [PYTHON2]
                        Run the tests with the interpreters specified by the
                        `interpreter` option, this is the default behavior,
                        but it must be called explicitly to run these
                        interpreters when Python 3 tests are run or the docs
                        are built (default: None)
  -3 [PYTHON3], --python3 [PYTHON3]
                        Run the tests with the interpreters specified by the
                        `interpreter3` configuration file option, which is
                        `python3` by default (default: False)
  -D [BUILD_DOCS], --build-docs [BUILD_DOCS]
                        Test the building of the Sphinx HTML documentation
                        (default: False)
  --no-comment          Upload review but do not submit summary comment to
                        pull request on GitHub (default: True)
  -i INTERPRETER, --interpreter INTERPRETER
                        Python interpreter used to run tests (default:
                        ['python'])
  --interpreter3 INTERPRETER3
                        Python 3 interpreter used to run tests (default:
                        ['python3'])
  -t COMMAND, --testcommand COMMAND
                        Command, run as an argument of `python`, used to
                        execute tests, allowing the use of sympy-bot on a
                        subset of the tests, to run a command that is not an
                        argument of `python`, add '-V;' to the beginning of
                        the option, for example '-V; mycommand' (default:
                        setup.py test)
  --build-docs-command COMMAND
                        Command run to build the Sphinx docs (default: make
                        html-errors)
  --build-docs-dir DIR  Directory in which to build the Sphinx docs (default:
                        doc)
  -r REFERENCE, --reference REFERENCE
                        Path to sympy repository, passed to 'git clone <sympy
                        repo>', setting this speeds up sympy-bot and decreases
                        network traffic (default: None)
  -m COMMIT, --master-commit COMMIT
                        Commit to use as master for merging, use 'HEAD' to not
                        merge (default: origin/master)
  -p {https,git}, --protocol {https,git}
                        Protocol for communicating with GitHub (default:
                        https)
  -s SERVER, --server SERVER
                        Server to upload results (default:
                        http://reviews.sympy.org)
  -R REPOSITORY, --repository REPOSITORY
                        GitHub repository used, allowing sympy-bot to be used
                        with other projects (default: sympy/sympy)
sympy-bot review: error: too few arguments

[asmeurer]

sympy-bot list is broken in master. See #122.

For the other error, with argparse, the pull request number has to come immediately after "review".

[certik]

Ah ok. In that case, this help is wrong:

usage: sympy-bot review [-h] [--profile PROFILE] [-n] [-2 [PYTHON2]]
                        [-3 [PYTHON3]] [-D [BUILD_DOCS]] [--no-comment]
                        [-i INTERPRETER] [--interpreter3 INTERPRETER3]
                        [-t COMMAND] [--build-docs-command COMMAND]
                        [--build-docs-dir DIR] [-r REFERENCE] [-m COMMIT]
                        [-p {https,git}] [-s SERVER] [-R REPOSITORY]
                        n [n ...]

The n needs to go right next to "review".

[asmeurer]

That must be a bug in argparse. We're just using the default functions from there.

On Aug 3, 2012, at 9:48 PM, "Ondřej Čertík" reply@reply.github.com wrote:

Ah ok. In that case, this help is wrong:

usage: sympy-bot review [-h] [--profile PROFILE] [-n] [-2 [PYTHON2]]
                       [-3 [PYTHON3]] [-D [BUILD_DOCS]] [--no-comment]
                       [-i INTERPRETER] [--interpreter3 INTERPRETER3]
                       [-t COMMAND] [--build-docs-command COMMAND]
                       [--build-docs-dir DIR] [-r REFERENCE] [-m COMMIT]
                       [-p {https,git}] [-s SERVER] [-R REPOSITORY]
                       n [n ...]

The n needs to go right next to "review".

---

Reply to this email directly or view it on GitHub: https://github.com/sympy/sympy-bot/pull/123#issuecomment-7498987

[asmeurer]

@Krastanov maybe instead of searching for shas, just keep track locally of which shas you've tested. It won't hurt if you test a PR that someone else has tested too (likely they are testing on a different configuration anyway).

[Krastanov]

@Krastanov maybe instead of searching for shas, just keep track locally of which shas you've tested. It won't hurt if you test a PR that someone else has tested too (likely they are testing on a different configuration anyway).

This is an option and I will probably do it. Anyway, the only reason for searching for shas was that many people were worried about excessive comments from the bot.

[certik]

I wouldn't worry about extensive comments.

[asmeurer]

Just pushed a commit that fixed a very subtle bug that was plaguing sympy/sympy#1462. Hopefully everything should work correctly now.

[asmeurer]

I'd like feedback before I do this. I just realized that we can save even more space. What if the output looked like this:

SymPy Bot Summary: :red_circle: There were test failures (merged mrocklin/trace (71ca3be7d64076064448d8210ba1f51063887cb7) into master (96259182f9fcc3a63609d401e412068e779618dc)). @mrocklin: Please fix the test failures. Interpreter 1: :eight_spoked_asterisk: All tests have passed: /sw/bin//python2.6 (2.6.7-final-0, Darwin, 64-bit) Interpreter 2: :red_circle: There were test failures: /sw/bin//python2.7 (2.7.3-final-0, Darwin, 64-bit) Interpreter 3: :red_circle: There were test failures: /sw/bin//python3.1 (3.1.5-final-0, Darwin, 64-bit) Interpreter 4: :eight_spoked_asterisk: All tests have passed: /sw/bin//python3.2 (3.2.3-final-0, Darwin, 64-bit) Build HTML Docs: :eight_spoked_asterisk: All tests have passed: Sphinx version: 1.1.2

[certik]

Yep, looks great to me.

[moorepants]

Looks good to me too. Cuts down a lot of space with little loss of info.

[asmeurer]

OK, done. I don't see how the summaries could be made any smaller now, without removing what is IMHO vital information, or making the formatting much harder to read.

[smichr]

Since which interpreter was used is obvious from what follows on a given line, the "Interpreter 1"-like prefixes could be removed so what would appear first are the emoji icons.

[asmeurer]

Right. I was also thinking of replacing Interpreter 1 with Python 3.2.3 (64-bit). And I'm not sure if the operating system is super important, so it can probably be removed (it's not even in the test header in the full report).

[asmeurer]

Regarding the order of the symbol, I did play with putting it before SymPy Bot (see for example https://github.com/asmeurer/GitHub-Issues-Test/issues/6#issuecomment-7473378), and it just didn't look good. I don't know how it would look for the interpreters, but probably it would look best the way it is now (after the bold header).

[smichr]

Regarding the emojis being first -- I think the header looks fine as is (with the emoji coming afterwards). I was talking about the individual results lines. It is there that I would omit the "Interpreter" information and just put the emoji first and the other info after it for each interpreter. You could perhaps remove the "All tests" text and just write "pass" or "fail".

Also, if you left a little space around the SHA numbers then it would be easier to copy one of them (to check out locally, for example).

[SymPy Bot][sympy-bot] Summary: :red_circle: There were test failures (merged mrocklin/trace ( 71ca3be7d64076064448d8210ba1f51063887cb7 ) into master ( 96259182f9fcc3a63609d401e412068e779618dc )). @mrocklin: Please fix the test failures. :eight_spoked_asterisk: pass: /sw/bin//python2.6 (2.6.7-final-0, Darwin, 64-bit) :red_circle: fail: /sw/bin//python2.7 (2.7.3-final-0, Darwin, 64-bit) :red_circle: fail: /sw/bin//python3.1 (3.1.5-final-0, Darwin, 64-bit) :eight_spoked_asterisk: pass: /sw/bin//python3.2 (3.2.3-final-0, Darwin, 64-bit) :eight_spoked_asterisk: pass: Sphinx version: 1.1.2

[asmeurer]

What about the fetch and merge failure text?

[asmeurer]

I think it would be better to do

:red_circle: Python 3.2.1 (64-bit): Fail

It's strange not seeing what's what, especially with the docs build.

[asmeurer]

Or

Python 3.2.1 (64-bit): :red_circle: Fail

[smichr]

On Thu, Aug 9, 2012 at 1:00 PM, Aaron Meurer notifications@github.comwrote:

Or

Python 3.2.1 (64-bit): [image: :red_circle:] Fail

My rationale is: I read left to right. The first thing I want to know is what failed or passed. Seeing the red (or green) I read to the right to see what it was. I usually am not looking to see if a particular version passed or failed. So what failed is less important than pass or fail (hence my desire to see the icon first).

[certik]

I do want to see the type of test that as run. That is, python version and either html, or a regular test run. Anyway, can we merge this soon? So that I can start using it.

[asmeurer]

Yeah, I'll fix it up as suggested. I also need to merge with master apparently.

[asmeurer]

OK, now see what you think.

[asmeurer]

I had to remove the part where I skipped the details for merge conflicts and fetch errors, because the url is in there, so it will require a little more refactoring. But I think we should not show that information in that case.

[certik]

@asmeurer, do you want us to test this more, or can we merge this?

[asmeurer]

If you are going to use SymPy bot in the next couple of days, go ahead and run it off this branch to test it. There have been several pretty bad bugs that I've fixed here (see the commit history). If not, just merge it, and we'll fix the bugs as they come up.

[asmeurer]

With that being said, barring any additional bugs, I'm done with my work here.

[certik]

Well, I am not, so let's merge it.

[smichr]

Not sure where to raise this. Perhaps the summary could say

SymPy Bot Summary: Failed when merging branch (sha1) into branch2 (sha2).
SymPy Bot Summary: Passed when merging branch1 (sha1) into branch2 (sha2).

Right now we have nested parentheses and the important verbs are listed off the email summary line length (at least on my screen).

I'll see if I can submit a pull request.

[asmeurer]

It should be quite easy to change the format. The relevant line of code is https://github.com/sympy/sympy-bot/blob/master/sympy-bot#L477. The new style string formatting really makes this a breeze.

[smichr]

I'm not sure why I'm not able to push the changes to sympy-bot. I forked sympy-bot at git and when I try to push a branch there I get

fatal: 'smichr' does not appear to be a git repository
fatal: The remote end hung up unexpectedly

Can you point me at what I have to do to get push access to there?

[moorepants]

It may have been the downtime github had today. status.github.com

On Thu, Aug 16, 2012 at 12:17 AM, Christopher Smith < notifications@github.com> wrote:

I'm not sure why I'm not able to push the changes to sympy-bot. I forked sympy-bot at git and when I try to push a branch there I get

fatal: 'smichr' does not appear to be a git repository fatal: The remote end hung up unexpectedly

Can you point me at what I have to do to get push access to there?

— Reply to this email directly or view it on GitHubhttps://github.com/sympy/sympy-bot/pull/123#issuecomment-7778381.

Personal Website http://biosport.ucdavis.edu/lab-members/jason-moore Sports Biomechanics Lab http://biosport.ucdavis.edu, UC Davis Davis Bike Collective http://www.davisbikecollective.org Minister, Davis, CA BikeDavis.info Google Voice: +01 530-601-9791 Home: +01 530-753-0794 Office: +01 530-752-2163 Lab: +01 530-752-2235

[asmeurer]

git remote add smichr git@github.com:smichr/sympy-bot.git