Add a method argument

[vindarel]

for #84

My new tests are failing, this in to start a discussion and welcome any remarks if you have some.

[abingham]

This approach looks just fine. As you say, the test is failing, but it seems to be failing in the appropriate way. Once you update the canned test case, I think the functionality will be fine.

I have a few comments on the code, but I'll make those inline.

[vindarel]

I had another look on the tests and I don't understand how to choose the method to add an argument to. For example, to projects.basic.a_free_func. Likewise I didn't get what the test_cross_remove_argument really works. Will try again.

[abingham]

I admit that the tests are more complicated than they need to be. The basic flow of test_cross_remove_argument is something like this:

1. withFixture first copies a few tests from the "projects" directory to the "active" directory (which is the directory that will be used by the tests)
2. The fixture also starts a project proxy. The Project is a pykka actor, so we need to start the actor before it can do anything.
3. Then the test calls "remove_argument" which takes a few arguments: the task_state set up by the fixture; the index of the argument to remove; the path in the main project to the file containing the function; and the distance into the file of the cursor (163, in this case).
4. The it uses "compare_projects" to compare "projects/cross_basic_remove_argument" with "active/main/basic". Essentially, "projects/cross_basic_remove_argument" is what we expect the projects to look like after refactoring, and "active/main/basic" is how it actually looks.
5. We repeat (4) for "projects/cross_use_bar_remove_argument" and "active/cross/bar".

So for your test, you'll need to do a few things:

1. Create some directories in "projects" containing the states of the projects you expect after refactoring.
2. Figure out the arguments you need to pass to add_argument needed to run your refactoring.
3. Basically do the rest of what test_cross_remove_argument does.

Again, I apologize for the current undocumented complexity of the tests. I put them together in a hurry, and I'd like to find time to rewrite them. Issue #87 is about rewriting them with pytest, and that would probably be good. But generally making them easier to understand would be good, too.

[abingham]

I've started the process of converting the test suite over to using pytest. It seems to be quite an improvement over the current version. I think it's a bit cleaner, so you might look at it for guidance.

In fact, if you want, you could add your new functionality on the master branch without tests. Then, we could add your tests to the pytest branch. I leave that up to you.

[abingham]

The pytest branch is complete now, but don't want to merge it in if it's going to derail or confuse your work. In that branch I've completely removed the unittest-based tests and am now using pytest universally.

If you don't mind, I can merge it in now and you can make your test using pytest. Or I can wait until you're done, then rebase the pytest stuff atop your changes and translate the new test to pytest. Which would you prefer?

[vindarel]

Please go ahead on merge pytest, it won't hurt my work (I'll adapt the non-test I have).

[vindarel]

Well, I thought I had rebased enough but that is not the case…

Anyway, I wanted to say I understand the tests with your explanations (the role of the offset), but it seems regular tests are failing so I don't know if mine is correct:

tests/test_rename.py .

==================================================== FAILURES ====================================================
_______________________________________________ test_get_children ________________________________________________

activate_package = <function f at 0x7f9813dc91b8>, start_project = <function f at 0x7f9813dc9578>

    def test_get_children(activate_package, start_project):
        activate_package(package='basic', into='main')
        proj = start_project('main')

>       assert (sorted(proj.get_children('basic').get()) ==
                [('basic/__init__.py', False),
                 ('basic/bar.py', False),
                 ('basic/foo.py', False),
                 ('basic/overrides.py', False)])

tests/test_project.py:18: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../.virtualenvs/traad/local/lib/python2.7/site-packages/pykka/threading.py:52: in get
    compat.reraise(*self._data['exc_info'])
../../.virtualenvs/traad/local/lib/python2.7/site-packages/pykka/compat.py:12: in reraise
    exec('raise tp, value, tb')
../../.virtualenvs/traad/local/lib/python2.7/site-packages/pykka/actor.py:201: in _actor_loop
    response = self._handle_receive(message)
../../.virtualenvs/traad/local/lib/python2.7/site-packages/pykka/actor.py:295: in _handle_receive
    return callee(*message['args'], **message['kwargs'])
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = Project("/home/vince/bacasable/traad/tests/active/main/"), path = 'basic'

    def get_children(self, path):
        '''Get a list of all child resources of a given path.

            ``path`` may be absolute or relative. If ``path`` is relative,
            then it must to be relative to the root of the project.

            Args:
              path: The path of the file/directory to query.

            Returns: A list of tuples of the form (path,
              is_folder).

            '''

>       path = self._to_relative_path(path)
E       AttributeError: 'Project' object has no attribute '_to_relative_path'

../../.virtualenvs/traad/local/lib/python2.7/site-packages/traad-0.11-py2.7.egg/traad/rope/project.py:171: AttributeError
=============================================== 1 tests deselected ===============================================
=============================== 1 failed, 24 passed, 1 deselected in 0.50 seconds ========================

[abingham]

@vindarel I've cherry-picked your changes cleanly onto master in the add-method-argument branch. Please have a look at that branch and let me know if accurately reflects the changes you've made. If so, then I'll merge it in and cancel this pull request.

Your work was good, but this cherry-pick was the simplest way for me to get a clean merge. I had to clean up a few things, but that was mostly a result of the confusing state of my code that you had to work with.

So let me know if you see anything in that branch that you don't agree with, and thanks again for your work on this. I know it wasn't particularly easy, but I really appreciate contributions like this!

[vindarel]

oh, so nice this cherry pick :) the changes look ok and the new test passes \o/ (but test_rename fails).

Now on emacs-traad ?

(noo it was very doable you had a solid architecture, and it was nice you letting me do )

[abingham]

Great! I'll sort this out right away.

And yes, it would be very nice if you could update emacs-traad, too. Thanks!