Hi,

I just posted three patches:

1. agr / rope / Pull request #3: Added noerror option to code_assist —
Bitbucket https://bitbucket.org/agr/rope/pull-request/3/added-noerror-option-to...

2. agr / ropemode / Pull request #3: Use noerror option for
code_assist — Bitbucket
https://bitbucket.org/agr/ropemode/pull-request/3/use-noerror-option-...

3. agr / ropemacs / Pull request #5: Added ropemacs-codeassist-noerror
— Bitbucket https://bitbucket.org/agr/ropemacs/pull-request/5/added-ropemacs-code...

The latter depends on the former.  The idea is to provide a silent way
to call rope API.  I think error must not be shown when rope is called
behind the user.  I want use this from auto-complet by dynamically
binding ropemacs-codeassist-noerror.

I attached the patches as they are rather big to paste in the mail.
The main part is this:

diff --git a/rope/contrib/codeassist.py b/rope/contrib/codeassist.py
--- a/rope/contrib/codeassist.py
+++ b/rope/contrib/codeassist.py
@@ -320,11 +324,17 @@
         return result

     def __call__(self):
         if self.offset > len(self.code):
             return []
-        completions = list(self._code_completions().values())
+        try:
+            completions = list(self._code_completions().values())
+        except exceptions.ModuleSyntaxError as err:
+            if self.noerror:
+                return []
+            else:
+                raise err
         if self.expression.strip() == '' and self.starting.strip() != '':
             completions.extend(self._matching_keywords(self.starting))
         return completions

     def _dotted_completions(self, module_scope, holding_scope):

Best,
Takafumi

rope.diff 4K Download

ropemode.diff 1K Download

ropemacs.diff < 1K Download

Hi Takafumi,

Agreed, showing the traceback is really annoying.

Thanks for including the patches and the summary.

Why not wrap the call in ropemode/interface.py, instead of changing
code_assist()?  Then newer versions of ropemode would continue to work
with older versions of rope; specially now that rope changes very little,
requiring new versions is somehow unexpected.  Also I think it would be
slightly simpler too.

Passing the exception to ropemode, also allows it to print a message
like "rope: unable to complete because of syntax errors; try increasing
maxfixes" or something shorter, if noerror is not set; it seems much
better than the exception traceback, at least for code assists; but I'm
not sure if it is a real improvement.

Anyhow, doesn't increasing the maxfixes help here?  I thought the
heuristics in rope/contrib/fixsyntax.py handle the common cases well.

Thanks,
Ali

On 2012-03-23 19:17 +0800, Ali Gholami Rudi wrote:

Is it general rule that the larger the maxfixes, the better? Why does it
default to 1?

I was wondering if it makes more sense that code assist ignores anything
after the point code assist is requested? For example CLANG¹ only parses
up to the point where code completion is requested so syntax errors
thereafter does not stop code completion from working.

Leo

Footnotes:
¹  http://clang.llvm.org
--
Your time is limited, so don’t waste it living someone else’s life.

Hi Ali,

On Fri, Mar 23, 2012 at 12:17 PM, Ali Gholami Rudi <aligr...@gmail.com> wrote:

Yes I think it is simpler solution. I didn't think about the version
issue. But as you can upload to pypi with changing the dependencies on
versions, I think it's OK. Users like the one who install from hg
repository can update them manually, I guess. Making better library
without breaking any backward compatibility is difficult...

The rationals of putting in rope module are: (1) to solve the problem
in the most upstream module so that it benefits other users other than
ropevim and ropemacs users; (2) to add unit test. it's easier to add
to it in rope since ropemode has the test only for dialog; (3) to be
more specific about the code to protect from errors.

I didn't think about the messaging.  Yes, it would be nice to have
short message with the pointer to how to fix. But I think having
no-error option is different problem. You can still have messaging
mechanism to catch rope-related errors in ropemode even if you have
noerror option. If ropemode has some logging layer, such that you can
change the logging level for messages, maybe catching errors in
ropemode is better.

I have 10 for maxfixes and I do not want to have very big number since
it blocks Emacs.  Also, you cannot be completely sure not having
errors by maxfixes.  I think it is better to have noerror option.

Best,
Takafumi

My understanding is that if maxfixes is large, you need more time to
get the response and that's why you need some balance.

Other orthogonal feature I want is some kind of caching so that you
can use the code which has no syntax errors and it would be quicker.
Running it in background of Emacs or Vim and having super quick auto
completion without errors might be very useful.

Takafumi

Hi Leo,

As Takafumi correctly pointed out, it would take longer and since it is
only a heuristic, it may not fix it even if it is run multiple times;
however I think it handles most common cases.  It certainly can be
improved, though.

The problem is when editing, one excepts to get completions for
definitions defined later in the file.  Like:

  void f():
      long_ [complete here...]

  long_variable1 = None
  long_variable2 = None

Also, the completion would fail, if a few statements before current
statement have syntax errors.  Anyway, fixsyntax.py is relatively
small and doesn't add much complexity.

        Ali

Hi,

But rope has changed so little in the last few years that such a change
would be quite unexpected.  What's more, what if ropevim is released
sooner than rope itself?  Or what if someone using rope library from
released packages, tries the repository version of ropevim/ropemode.

I have no problem with breaking backward (or forward) compatibility
for big releases with major changes, and we can mention it in
the change logs explicitly.  But now it doesn't seem a good idea.

But the propagation of changes makes me a bit worried as all files
involved in the path, depend on the new interface (from plugins, ropemacs,
to helper modules, ropemode, and the core, rope).

About (1), very little is shared.  Changing _calculate_proposals()
in ropemode to handle the noerror option itself:

  try:
      assist = _PythonCodeAssist(...)
  expect exceptions.ModuleSyntaxError:
      # output an error message
      self.env.message("rope: completion failed due to syntax errors")
      # or exit silently
      return []
  return assist()

About (2), that IS a disadvantage, but we don't have tests for
most of the UI anyway.  For (3), I think the UI should decide what
to do in error conditions: it can ignore it, it can print a more
civilized error message, or it can ask if the user wishes to see
the traceback for debugging.  But whatever, I think the decision
and the handling belong to the UI, and handling the error in
rope seems unnecessary.

We do handle errors in local_command and global_command hooks
in ropemode, which users can monkey patch.

Thanks,
Ali

Hi Ali,

On Sat, Mar 24, 2012 at 5:57 AM, Ali Gholami Rudi <aligr...@gmail.com> wrote:

My change on rope does not break backward compatibility, as I put the
option at the last position in the keyword argument.  And the change
on ropemacs does not break it neither, because it just adds a
variable.  Only the change on ropemode breaks backward compatibility,
as it assumes that code_assist has the new noerror option.

I think breaking compatibility in the development version is OK to do.
But anyway, I will move the code in ropemode.

What do you mean by very little is shared?  I thought there are some
IDEs using rope. You mean they do not use code_assist?

I think you need to put assist() in the try-clause, as it is where the
error is raised.  I will change
ropemode.interface._CodeAssist._calculate_proposals like this:

    try:
        proposals = codeassist.code_assist(
            self.interface.project, self.source, self.offset,
            resource, maxfixes=maxfixes)
    expect exceptions.ModuleSyntaxError as err:
        if noerror:
            proposals = []
        else:
            raise err

OK, I was not sure about who should handle the error.  That makes me
comfortable.

Oh, I didn't know that. So _CodeAssist._calculate_proposals should
raise error when the noerror option is False instead of showing the
message like you did, right?

Best,
Takafumi

Hi,

Yes, it is more of forward compatibility.  But an annoying one;
as I mentioned before, if ropevim is released sooner than rope,
we're in deep trouble.

I mean the amount of code reused by adding the noerror argument to
_PythonCodeAssist() is very small and not worth the effort.  Plus,
ropemode is a better place to decide about error conditions.

You're right.

Looks good.  How about printing an error message instead of re-raising the
exception in the !noerror case? For code-assists, it may be acceptable.

I think in the last resort, yes; that is for exceptions we have
little idea why they happened.  For others, like input errors in
refactorings, printing a more meaningful error message may be preferable.
For code-assists, syntax errors are quite common so we may just say rope
failed to fix the syntax errors.  Better ideas?  Anyhow, AFAIR at the
moment all error messages are handled through ropemode decorators.

Thanks,
Ali

Hi,

On Sat, Mar 24, 2012 at 2:00 PM, Ali Gholami Rudi <aligr...@gmail.com> wrote:

I see that _exception_handler in ropemode.decorators catches
ModuleSyntaxError and _exception_message in the same module prints
(via message function) the error.  I think this is the place to change
if you want to add meaningful error messages and having them not only
for completion but also for all kind of operations.  Or do you want to
notify user the context of the error?  Your suggestion "rope:
completion failed due to syntax errors" certainly contains that
information.

Best,
Takafumi

On the other hand, most of the good bug reports we had in the past were
accompanied with tracebacks; it speeds up identifying bugs.  So, hiding
the traceback for everything doesn't seem a good idea.

But on the other hand, errors which are probably bugs should
happen rarely.  Usually we get input errors:

* Syntax errors when completing is probably safe to ignore;
  we can show a message instead of the traceback; so I like
  your revised patch.
* All RefactoringErrors are safe to ignore their traceback,
  AFAIR.  Usually they report bad inputs; I grepped the files
  in rope/refactor/ for RefactoringError messages:

  + Unresolvable name selected
  + Use function works for global functions, only
  + Move only works on classes, functions, modules and methods.
  + Only normal methods can be moved.
  + ...

I just had another look at Logger in ropemode/decorators.py; setting
only_short (ropemode.decorators.logger.only_short = True) tells ropemode
not to print the traceback for three types of exceptions.  I wonder
why it has not been used in ropemode users.  How about introducing a
configuration variable for it in ropevim, which defaults to True?

Thanks,
Ali

Hi,

On Sat, Mar 24, 2012 at 8:50 PM, Ali Gholami Rudi <aligr...@gmail.com> wrote:

Agree.

My patch does not hide traceback when the completion is called
manually (e.g., using M-/) if user set False for noerror.

This was my suggestion:

What is your choice?:

(1) ignore all error (pass in the except-clause) and remove noerror option.
(2) my suggestion, as is.
(3) keep the noerror option like that, but show short message instead
of raise err (your first suggestion).
(4) same as (2), but default of noerror is True.
(5) same as (3), but default of noerror is True.

The problem is "short" message is not that short. When I use
`only_short = True` and try completion, I got:

ModuleSyntaxError: Syntax error in file <docutils/nodes.py> line <37>:
Syntax errors in file docutils/nodes.py:
  * line 35: invalid syntax ... fixed
  * line 37: unexpected indent ... fixed
  * line 37: unexpected indent ... fixed
  * line 37: unexpected indent ... fixed
  * line 37: unexpected indent ... fixed
  * line 37: unexpected indent ... fixed
  * line 37: unexpected indent ... fixed
  * line 37: unexpected indent ... fixed
  * line 37: unexpected indent ... fixed
  * line 37: unexpected indent ... raised!

I have 10 lines because I have 10 for maxfixes.  To make it to fit in
one line (and maybe less than 80 characters), we need to modify
decorators._exception_message.

To make only_short configurable, I suggest to set env attribute to
logger like this:

class RopeMode(object):

    def __init__(self, env):
        decorators.logger.env = env
        ...

And in logger, you can bypass self.env.get('only_short_message') (or
something like that) to self.only_short using @property.

Takafumi

Hi Takafumi,

I prefer (2) in addition to modifying logger.only_short based
on the value of noerror.

I see.  But I think these long error messages are rare; maybe we
can only print the last line.

By the way, how easy is the offending source code easy to fix?  If it is
a common case and easy, we can probably change fixsyntax.py to handle it.

Agreed.  It is not that clean but I cannot think of anything
better at the moment.

Thanks,
Ali

Ali Gholami Rudi <aligr...@gmail.com> wrote:

Now that I think about it, I think they should be two separate
options:

* Ideally there should be a "brieferrors" option; we only show the
  traceback in the logger when it is not set.  In the code-assist
  case, if it is not set, we can re-raise it, instead of printing
  the error message.
* The noerror option, as far as I can say, is useful for ropemode
  functions, not commands; functions like RopeMode.completions()
  can return an empty output (like the empty list in your example)
  if an error occurs when noerror is set.

What do you think?

        Ali

Hi Ail,

On Sun, Mar 25, 2012 at 5:36 AM, Ali Gholami Rudi <aligr...@gmail.com> wrote:

Good. So I will send a pull request for this and later for only_short.

In my case, it was *always* like that, since if the fixing is failed
that means there were 10 failures. I think showing exception name and
the last line is OK for now, but better way is to have short_message
method for all rope exception and use it for the short message. I
don't know if showing the last line for any error message is the right
answer, exception class itself knows what is the best answer and it
can store the information in a way to help creating short message
rather than "parsing" (split by newlines) afterwards. What do you
think?

I have no idea.  I will have a look at it sometimes later.

Slightly cleaner way to do it is to add Logger.set_env method. What do
you think?

Best,
Takafumi

Hi Ali,

On Sun, Mar 25, 2012 at 6:06 AM, Ali Gholami Rudi <aligr...@gmail.com> wrote:

Hmm... I don't know. Other way to do it is to have "*rope log*" buffer
and inserting traceback all the time in background. I think showing
multiline message such as traceback in the minibuffer (in Emacs; I
don't the name for vim equivalent) should be avoided always.  Note
that even Emacs shows lisp traceback in a different buffer.  And it is
a little bit easier to find the traceback than in the *Message*
buffer. We can even ask user to save the buffer to a file and attach
it to mail.

Best,
Takafumi

Hi Ali,

I just posted the request:
https://bitbucket.org/agr/ropemode/pull-request/4/

diff -r bd969e488130 -r 1e251abf45c2 ropemode/interface.py
--- a/ropemode/interface.py     Wed Feb 08 12:16:43 2012 +0800
+++ b/ropemode/interface.py     Sun Mar 25 10:01:31 2012 +0200
@@ -626,13 +626,19 @@

     def _calculate_proposals(self):
         self.interface._check_project()
         resource = self.interface.resource
         maxfixes = self.env.get('codeassist_maxfixes')
-        proposals = codeassist.code_assist(
-            self.interface.project, self.source, self.offset,
-            resource, maxfixes=maxfixes)
+        try:
+            proposals = codeassist.code_assist(
+                self.interface.project, self.source, self.offset,
+                resource, maxfixes=maxfixes)
+        except exceptions.ModuleSyntaxError as err:
+            if self.env.get('codeassist_noerror'):
+                proposals = []
+            else:
+                raise err
         if self.env.get('sorted_completions', True):
             proposals = codeassist.sorted_proposals(proposals)
         if self.autoimport is not None:
             if self.starting.strip() and '.' not in self.expression:
                 import_assists = self.autoimport.import_assist(self.starting)

Best,
Takafumi

Hi Takafumi,

Thanks.  Only one suggestion: as I mentioned in my previous mail, noerror
probably makes sense only for functions, like ropevim_completions, not
for commands like ropevim_codeassist.  Almost always, users prefer a
message like "rope failed to ..."  when there are errors, rather than an
empty response which may mean no completions.  For commands, something
like brieferrors makes more sense, though that's another story.

So I suggest moving the try-expect block to the functions calling
_calculate_proposals(), or passing a noerror argument to it, which
is true only in functions.  Like completions() and extended_completions().

Thanks,
Ali

On Sun, Mar 25, 2012 at 1:12 PM, Ali Gholami Rudi <aligr...@gmail.com> wrote:

Here you go!
https://bitbucket.org/agr/ropemode/pull-request/5/

If it is just for completions() and extended_completions(), isn't it
better to use argument instead of a global variable?  My aim was to
provide a global way to control the error messages because you can
call any commands and functions from outside of ropemode and caller
may want to suppress the errors.

What is the use case you have in mind? (1) rope user may set the
default for noerror option but the external module can override the
default by dynamically changing it; (2) rope users set the value and
external module should not alter the value. if the users want to see
the errors while calling completions via the external module, let them
see it.

I have (1) in mind, so having argument for that makes more sense, if
you really want to distinguish APIs (completions and
extended_completions) and interactive commands.  But if (2) is the
goal, having global variable for it is better. I like the idea of (1)
because user benefits a lot without a special setup.

Best,
Takafumi

ACK.

The main use case I was thinking about was, an emacs completion
module may collect completions from many sources and it may call
ropemacs-completions in elisp to get the list.  What we have at
the moment, interrupts the completions if there are unfixable
syntax errors.  With this new option, there would be no ropemacs
completions, but the completion module may find other completions,
for instance from the words typed before.  Probably something
similar exists for vim.

I'm quite happy with your new patch.  One can control errors in,
what you correctly call, the ropevim API.  So one can change
the behavior with ropemacs-noerror or ropevim_noerror.

Of course it can be further improved: we can change the localcommand
decorator to take a "default" argument.  For functions that pass a value
for this argument, based on the value of noerror option, we can return
the "default" instead of throwing rope-specific exceptions.  Is this
what you meant?  It seems a good idea.

Thanks,
Ali

On 2012-03-25 19:12 +0800, Ali Gholami Rudi wrote:

For emacs in
https://github.com/leoliu/Pymacs/commit/af0d659e8f6ef1244b3e7655b8e3d...,
I made pymacs respect debug-on-error, i.e. it doesn't return the whole
backtrace if debug-on-error is nil.

When there is no completion, signal an error is good but a multi-line
error message is not because it causes the minibuffer to enlarge which
is visually disturbing.

Leo

Hi,

On Sun, Mar 25, 2012 at 3:21 PM, Ali Gholami Rudi <aligr...@gmail.com> wrote:

So it's OK to external modules to alter noerror value temporary
(dynamical binding in Emacs, but I guess vim can do the same), right?
I mean, this is the purpose of this change.

What do you mean by default argument?  Something like this?:

    def completions(self, default=None):
        proposals = self._calculate_proposals(...)
        if not proposals and default is not None:
            return default
        else:
            return proposals

What I meant was this:

    def completions(self, noerror=None):
        proposals = self._calculate_proposals(noerror=noerror)
        return proposals

Best,
Takafumi

Great change!

Yes, that's exactly why I wanted to send the patches.

Best,
Takafumi

All RopeMode interface methods are either global commands,
like open project, or local commands, like refactorings.  The
local_command and global_command decorators declare these methods;
see ropemode/decorators.py.  It already has many arguments; like "key"
for specifying its default keybinding.  local_command() can be changed
so that if the "default" argument is given a new exception wrapper
(see _exception_handler in ropemode/decorators.py) is used that, based
on noerror, either returns the default value or raises the exception.
So we would have:

  @decorators.local_command(default=[])
  def completions(self):
      ...

The benefit of this change is it is very clean and can be easily added
to all ropemode API functions.  But frankly, I'm not sure how users
feel about this change.  If very few would ever need this, it seems like
unneeded complexity.  Leo's patch, on the other hand, seem to be related
to the other (my brieferrors suggestion) option.  Is there anyone who
thinks this is a very useful feature that we missed a lot and plans to
use it right away?

        Ali

I think it is better to change rope/contrib/fixerror.py, since AFAIR,
it is the only place we use multi-line short exception messages.

        Ali