Patches: noerror option to code_assist

Showing 1-49 of 49 messages
Patches: noerror option to code_assist Takafumi Arakaki 3/23/12 3:54 AM
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-code_assist

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

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

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

Re: Patches: noerror option to code_assist aligrudi 3/23/12 4:17 AM
Hi Takafumi,

Takafumi Arakaki <aka...@gmail.com> wrote:
> 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.

Agreed, showing the traceback is really annoying.

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

Thanks for including the patches and the summary.

> --- a/rope/contrib/codeassist.py
> +++ b/rope/contrib/codeassist.py
>  def code_assist(project, source_code, offset, resource=None,
> -                templates=None, maxfixes=1, later_locals=True):
> +                templates=None, maxfixes=1, later_locals=True, noerror=False):

> --- a/ropemode/interface.py
> +++ b/ropemode/interface.py
> +        noerror = self.env.get('codeassist_noerror')
>          proposals = codeassist.code_assist(
>              self.interface.project, self.source, self.offset,
> -            resource, maxfixes=maxfixes)
> +            resource, maxfixes=maxfixes, noerror=noerror)

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

Re: Patches: noerror option to code_assist Leo 3/23/12 10:27 AM
On 2012-03-23 19:17 +0800, Ali Gholami Rudi wrote:
> Anyhow, doesn't increasing the maxfixes help here?  I thought the
> heuristics in rope/contrib/fixsyntax.py handle the common cases well.

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.

> Thanks,
> Ali

Leo

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

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/23/12 11:15 AM
Hi Ali,

On Fri, Mar 23, 2012 at 12:17 PM, Ali Gholami Rudi <alig...@gmail.com> wrote:
> 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.

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.

> 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.

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.


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

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

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/23/12 12:00 PM
On Fri, Mar 23, 2012 at 6:27 PM, Leo <sdl...@gmail.com> wrote:
> On 2012-03-23 19:17 +0800, Ali Gholami Rudi wrote:
>> Anyhow, doesn't increasing the maxfixes help here?  I thought the
>> heuristics in rope/contrib/fixsyntax.py handle the common cases well.
>
> Is it general rule that the larger the maxfixes, the better? Why does it
> default to 1?

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


> 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.

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

Re: Patches: noerror option to code_assist aligrudi 3/23/12 9:45 PM
Hi Leo,

Leo <sdl...@gmail.com> wrote:
> Is it general rule that the larger the maxfixes, the better? Why does it
> default to 1?

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.

> 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.

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

Re: Patches: noerror option to code_assist aligrudi 3/23/12 9:57 PM
Hi,

Takafumi Arakaki <aka...@gmail.com> wrote:
> On Fri, Mar 23, 2012 at 12:17 PM, Ali Gholami Rudi <alig...@gmail.com> wrote:
> > 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.
>
> 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...

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.

> 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.

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.

> > 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.
>
> 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.

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

Thanks,
Ali

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/24/12 5:07 AM
Hi Ali,

On Sat, Mar 24, 2012 at 5:57 AM, Ali Gholami Rudi <alig...@gmail.com> wrote:
> 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.

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.

>> 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.
>
> 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()

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


> 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.

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


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

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

Re: Patches: noerror option to code_assist aligrudi 3/24/12 6:00 AM
Hi,

Takafumi Arakaki <aka...@gmail.com> wrote:
> On Sat, Mar 24, 2012 at 5:57 AM, Ali Gholami Rudi <alig...@gmail.com> wrote:
> > 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.
>
> 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.

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.

> > 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()
>
> 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 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.

> I think you need to put assist() in the try-clause, as it is where the
> error is raised.  I will change

You're right.

> 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

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.

> > We do handle errors in local_command and global_command hooks
> > in ropemode, which users can monkey patch.
>
> 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?

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

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/24/12 8:18 AM
Hi,

On Sat, Mar 24, 2012 at 2:00 PM, Ali Gholami Rudi <alig...@gmail.com> wrote:
> 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.

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

Re: Patches: noerror option to code_assist aligrudi 3/24/12 12:50 PM

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

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/24/12 3:13 PM
Hi,

On Sat, Mar 24, 2012 at 8:50 PM, Ali Gholami Rudi <alig...@gmail.com> wrote:
> 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.

Agree.


> 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.

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:

>    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

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.


> * 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?

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

Re: Patches: noerror option to code_assist aligrudi 3/24/12 8:36 PM
Hi Takafumi,

Takafumi Arakaki <aka...@gmail.com> wrote:
> > * Syntax errors when completing is probably safe to ignore;
> > �we can show a message instead of the traceback; so I like
> > �your revised patch.
>
> My patch does not hide traceback when the completion is called
> manually (e.g., using M-/) if user set False for noerror.
>
> 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.

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

> > 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?
>
> 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 ... raised!

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

> 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.

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.

> 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.

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

Thanks,
Ali

Re: Patches: noerror option to code_assist aligrudi 3/24/12 9:06 PM
Ali Gholami Rudi <alig...@gmail.com> wrote:
> Takafumi Arakaki <aka...@gmail.com> wrote:
> > My patch does not hide traceback when the completion is called
> > manually (e.g., using M-/) if user set False for noerror.
> >
> > 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.
>
> I prefer (2) in addition to modifying logger.only_short based
> on the value of noerror.

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

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/25/12 12:41 AM
Hi Ail,

On Sun, Mar 25, 2012 at 5:36 AM, Ali Gholami Rudi <alig...@gmail.com> wrote:
> Hi Takafumi,
>
> Takafumi Arakaki <aka...@gmail.com> wrote:
>> > * Syntax errors when completing is probably safe to ignore;
>> >  we can show a message instead of the traceback; so I like
>> >  your revised patch.
>>
>> My patch does not hide traceback when the completion is called
>> manually (e.g., using M-/) if user set False for noerror.
>>
>> 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.
>
> I prefer (2) in addition to modifying logger.only_short based
> on the value of noerror.

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

>> 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 ... raised!
>
> I see.  But I think these long error messages are rare; maybe we
> can only print the last line.

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?


> 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.

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


>> 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.
>
> Agreed.  It is not that clean but I cannot think of anything
> better at the moment.

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


Best,
Takafumi

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/25/12 12:54 AM
Hi Ali,

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

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/25/12 1:21 AM
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

Re: Patches: noerror option to code_assist aligrudi 3/25/12 4:12 AM
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

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/25/12 5:37 AM

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

Re: Patches: noerror option to code_assist aligrudi 3/25/12 6:21 AM
Takafumi Arakaki <aka...@gmail.com> wrote:
> Here you go!
> https://bitbucket.org/agr/ropemode/pull-request/5/

ACK.

> 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.

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 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.

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

Re: Patches: noerror option to code_assist Leo 3/25/12 7:00 AM
On 2012-03-25 19:12 +0800, Ali Gholami Rudi wrote:
> 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.

For emacs in
https://github.com/leoliu/Pymacs/commit/af0d659e8f6ef1244b3e7655b8e3ddcfba4f128b,
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

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/25/12 9:50 AM
Hi,

On Sun, Mar 25, 2012 at 3:21 PM, Ali Gholami Rudi <alig...@gmail.com> wrote:
>> 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.
>
> 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.

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.


> 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.


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

Re: Patches: noerror option to code_assist Takafumi Arakaki 3/25/12 9:54 AM
On Sun, Mar 25, 2012 at 4:00 PM, Leo <sdl...@gmail.com> wrote:
> On 2012-03-25 19:12 +0800, Ali Gholami Rudi wrote:
>> 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.
>
> For emacs in
> https://github.com/leoliu/Pymacs/commit/af0d659e8f6ef1244b3e7655b8e3ddcfba4f128b,
> I made pymacs respect debug-on-error, i.e. it doesn't return the whole
> backtrace if debug-on-error is nil.

Great change!


> 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.

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


Best,
Takafumi

Re: Patches: noerror option to code_assist aligrudi 3/25/12 12:28 PM
Takafumi Arakaki <aka...@gmail.com> wrote:
> > 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.
>
>
> 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

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

Re: Patches: noerror option to code_assist aligrudi 3/25/12 12:38 PM
Takafumi Arakaki <aka...@gmail.com> wrote:
> > 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.
>
> Yes, that's exactly why I wanted to send the patches.

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

Re: Patches: noerror option to code_assist Anton Beloglazov 8/22/12 11:02 PM
Hi All,

Sorry, I couldn't understand from the discussion how this issue can be fixed in Emacs. Currently, I'm getting the following kind of errors in the expanded minibuffer when there are some syntax errors:

ModuleSyntaxError: Syntax error in file <tests/test_collector.py> line <266>: 
Syntax errors in file tests/test_collector.py:
  * line 266: unexpected indent ... fixed
  * line 266: unexpected indent ... fixed
  * line 266: unexpected indent ... fixed
  * line 266: unexpected indent ... fixed
  * line 266: unexpected indent ... fixed
  * line 266: unexpected indent ... fixed
  * line 266: unexpected indent ... fixed
  * line 266: unexpected indent ... fixed
  * line 266: unexpected indent ... fixed
  * line 266: unexpected indent ... raised!

Could you please clarify for me how these errors can be hidden or reduced to one line in Emacs?

Thanks,
Anton

On Monday, 26 March 2012 06:38:42 UTC+11, aligrudi wrote:
Takafumi Arakaki <aka...@gmail.com> wrote:
> > 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.
>
> Yes, that's exactly why I wanted to send the patches.

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

Re: Patches: noerror option to code_assist aligrudi 8/23/12 6:59 AM
Hi,

Anton Beloglazov <anton.be...@gmail.com> wrote:
> Sorry, I couldn't understand from the discussion how this issue can be
> fixed in Emacs. Currently, I'm getting the following kind of errors in the
> expanded minibuffer when there are some syntax errors:

I meant something like this patch for rope.  Not tested, so it
may need to be tweaked to work.

        Ali

--- a/rope/contrib/fixsyntax.py        2012-08-23 18:21:45.510172364 +0430
+++ b/rope/contrib/fixsyntax.py        2012-08-23 18:28:15.150743776 +0430
@@ -15,7 +15,7 @@
     @utils.saveit
     def get_pymodule(self):
         """Get a `PyModule`"""
-        errors = []
+        msg = None
         code = self.code
         tries = 0
         while True:
@@ -27,19 +27,15 @@
                 return self.pycore.get_string_module(
                     code, resource=self.resource, force_errors=True)
             except exceptions.ModuleSyntaxError, e:
+                if msg is None:
+                    msg = "%s:%s %s" % (e.filename, e.lineno, e.message_)
                 if tries < self.maxfixes:
                     tries += 1
                     self.commenter.comment(e.lineno)
                     code = '\n'.join(self.commenter.lines)
-                    errors.append('  * line %s: %s ... fixed' % (e.lineno,
-                                                                 e.message_))
                 else:
-                    errors.append('  * line %s: %s ... raised!' % (e.lineno,
-                                                                   e.message_))
-                    new_message = ('\nSyntax errors in file %s:\n' % e.filename) \
-                                   + '\n'.join(errors)
                     raise exceptions.ModuleSyntaxError(e.filename, e.lineno,
-                                                       new_message)
+                                 'Failed to fix error: %s' % msg)
 
     @property
     @utils.saveit

Re: Patches: noerror option to code_assist Leo 8/23/12 4:41 PM
On 2012-08-23 21:59 +0800, Ali Gholami Rudi wrote:
> I meant something like this patch for rope.  Not tested, so it
> may need to be tweaked to work.

Note that Pymacs (0.25 and above) no longer prints the whole backtrace
unless debug-on-error is non-nil.

Leo

Re: Patches: noerror option to code_assist Anton Beloglazov 8/23/12 7:03 PM
Thanks for your replies! Is there any issue with incorporating this fix into the main branch of rope?

Thanks,
Anton
Re: Patches: noerror option to code_assist aligrudi 8/23/12 7:09 PM
Anton Beloglazov <anton.be...@gmail.com> wrote:
> Thanks for your replies! Is there any issue with incorporating this fix
> into the main branch of rope?

I think that's a good idea.  Did you test it?

> On Friday, 24 August 2012 09:41:44 UTC+10, Leo wrote:
> > Note that Pymacs (0.25 and above) no longer prints the whole backtrace
> > unless debug-on-error is non-nil.

I don't think that would help, since we do want the error
message but the message is too long; codeassist syntax
errors have multi-line messages, each line describing an
attempt to fix the error.  The patch in my previous
mail made it less verbose.

        Ali

Re: Patches: noerror option to code_assist Anton Beloglazov 8/27/12 6:13 AM
Hi Ali,

Sorry for the delay. I've tested your patch: the error message has got indeed reduced into one line. However, the mini buffer still expands into multiple lines for some reason. Here is a screenshot: http://i.imgbox.com/ady0ackX.jpg

Do you know what could be the reason for such behavior?

Thanks,
Anton
Re: Patches: noerror option to code_assist aligrudi 8/27/12 8:05 AM
Hi,

Anton Beloglazov <anton.be...@gmail.com> wrote:
> Sorry for the delay. I've tested your patch: the error message has got
> indeed reduced into one line. However, the mini buffer still expands into
> multiple lines for some reason. Here is a screenshot:
> http://i.imgbox.com/ady0ackX.jpg
>
> Do you know what could be the reason for such behavior?

Odd...  Does the included patch make any difference.

        Ali

--- a/rope/contrib/fixsyntax.py        2012-08-27 19:42:30.516015457 +0430
+++ b/rope/contrib/fixsyntax.py        2012-08-27 19:45:00.236235582 +0430
@@ -15,7 +15,7 @@
     @utils.saveit
     def get_pymodule(self):
         """Get a `PyModule`"""
-        errors = []
+        msg = None
         code = self.code
         tries = 0
         while True:
@@ -27,19 +27,14 @@
                 return self.pycore.get_string_module(
                     code, resource=self.resource, force_errors=True)
             except exceptions.ModuleSyntaxError, e:
+                if msg is None:
+                    msg = '%s:%s %s' % (e.filename, e.lineno, e.message_.rstrip())
                 if tries < self.maxfixes:
                     tries += 1
                     self.commenter.comment(e.lineno)
                     code = '\n'.join(self.commenter.lines)
-                    errors.append('  * line %s: %s ... fixed' % (e.lineno,
-                                                                 e.message_))
                 else:
-                    errors.append('  * line %s: %s ... raised!' % (e.lineno,
-                                                                   e.message_))
-                    new_message = ('\nSyntax errors in file %s:\n' % e.filename) \
-                                   + '\n'.join(errors)
-                    raise exceptions.ModuleSyntaxError(e.filename, e.lineno,
-                                                       new_message)
+                    raise exceptions.ModuleSyntaxError(e.filename, e.lineno, msg)
 
     @property
     @utils.saveit

Re: Patches: noerror option to code_assist Anton Beloglazov 8/27/12 6:42 PM
Thanks, Ali. Unfortunately, this has not fixed the problem. Here is a new screenshot: http://i.imgbox.com/abvW3Bjc.jpg

Best regards,
Anton
Re: Patches: noerror option to code_assist Anton Beloglazov 8/27/12 7:26 PM
Hi Ali,

I've just found that this message is popped up by eldoc-mode. I'm using rope-get-calltip as eldoc-documentation-function. Due to syntax errors, it failed all the time and displayed that message. To fix this, I've modified the get_calltip function in rope/contrib/codeassist.py. Here is the patch:

--- codeassist.py 2012-08-28 12:17:06.801540893 +1000
+++ codeassist-patched.py 2012-08-28 12:12:31.444884763 +1000
@@ -90,7 +90,10 @@
     """
     fixer = fixsyntax.FixSyntax(project.pycore, source_code,
                                 resource, maxfixes)
-    pymodule = fixer.get_pymodule()
+    try:
+        pymodule = fixer.get_pymodule()
+    except exceptions.ModuleSyntaxError:
+        return None
     pyname = fixer.pyname_at(offset)
     if pyname is None:
         return None

Now it just displays nothing if there is a syntax error, which I think is reasonable for this purpose. Otherwise, it continuously shows up error messages, which is very annoying.

Thanks,
Anton 
Re: Patches: noerror option to code_assist Anton Beloglazov 8/28/12 12:26 AM
I had to make a similar fix in the _code_completions function to suppress the error reporting during auto-completion. The updated patch is below:

--- codeassist.py 2012-08-28 17:20:18.937734789 +1000
+++ codeassist-backup.py 2012-08-28 12:17:06.801540893 +1000
@@ -90,10 +90,7 @@
     """
     fixer = fixsyntax.FixSyntax(project.pycore, source_code,
                                 resource, maxfixes)
-    try:
-        pymodule = fixer.get_pymodule()
-    except exceptions.ModuleSyntaxError:
-        return None
+    pymodule = fixer.get_pymodule()
     pyname = fixer.pyname_at(offset)
     if pyname is None:
         return None
@@ -393,10 +390,7 @@
         lineno = self.code.count('\n', 0, self.offset) + 1
         fixer = fixsyntax.FixSyntax(self.pycore, self.code,
                                     self.resource, self.maxfixes)
-        try:
-            pymodule = fixer.get_pymodule()
-        except exceptions.ModuleSyntaxError:
-            return {}
+        pymodule = fixer.get_pymodule()
         module_scope = pymodule.get_scope()
         code = pymodule.source_code
         lines = code.split('\n')

Thanks,
Anton
Re: Patches: noerror option to code_assist aligrudi 8/28/12 6:19 AM
Hi Anton,

Anton Beloglazov <anton.be...@gmail.com> wrote:
> I had to make a similar fix in the _code_completions function to suppress
> the error reporting during auto-completion. The updated patch is below:

I'm wondering if it is a good idea to make a distinction between
ropemode commands and functions; for instance between rope-show-doc
and rope-get-doc; the former is a command in ropemacs/ropevim, while
the latter is a function that can be used in external commands.  If an
error happens when performing a command, I think the user expects an
error message (my fixsyntax.py patch makes one such error more compact,
so I think it would be still useful).  But functions are invoked from
elisp/vimscript and instead of breaking the call chain by throwing
an exception, I think it is a better idea to return a special value.
This is what your patches did, but I think that could be improved.

At the moment both commands and functions are specified using the
local_command decorator in ropemode/interface.py.  I suggest adding a
local_function decorator in ropemode/decorators.py whose only argument is
the value to return if an interrupt occurs, as in the included untested
patch.  What do you think?  Is the default return value of completions
and extended_completions acceptable?

Thanks,
Ali

diff -u a/ropemode/decorators.py b/ropemode/decorators.py
--- a/ropemode/decorators.py        2012-08-28 18:05:56.132361619 +0430
+++ b/ropemode/decorators.py        2012-08-28 18:14:21.633103214 +0430
@@ -47,13 +47,17 @@
                     exceptions.ModuleSyntaxError,
                     exceptions.BadIdentifierError)
 
-def _exception_handler(func):
+log_errors = object()
+
+def _exception_handler(func, error_return):
     def newfunc(*args, **kwds):
         try:
             return func(*args, **kwds)
         except exceptions.RopeError, e:
             short = None
             if isinstance(e, input_exceptions):
+                if error_return is not log_errors:
+                    return error_return
                 short = _exception_message(e)
             logger(str(traceback.format_exc()), short)
     newfunc.__name__ = func.__name__
@@ -72,10 +76,10 @@
         return func
     return decorator
 
-
-def local_command(key=None, prefix=False, shortcut=None, name=None):
+def local_command(key=None, prefix=False, shortcut=None,
+                  name=None, error_return=log_errors):
     def decorator(func, name=name):
-        func = _exception_handler(func)
+        func = _exception_handler(func, error_return)
         func.kind = 'local'
         func.prefix = prefix
         func.local_key = key
@@ -86,10 +90,12 @@
         return func
     return decorator
 
+def local_function(error_return=None):
+        local_command(error_return=error_return)
 
 def global_command(key=None, prefix=False):
     def decorator(func):
-        func = _exception_handler(func)
+        func = _exception_handler(func, error_return)
         func.kind = 'global'
         func.prefix = prefix
         func.global_key = key
diff -u ropemode_/interface.py ropemode/interface.py
--- ropemode_/interface.py        2012-08-28 18:05:56.132361619 +0430
+++ ropemode/interface.py        2012-08-28 18:20:43.313655977 +0430
@@ -157,7 +157,7 @@
     def pop_mark(self):
         self.env.pop_mark()
 
-    @decorators.local_command()
+    @decorators.local_function()
     def definition_location(self):
         definition = self._base_definition_location()
         if definition:
@@ -200,7 +200,7 @@
         else:
             self.env.message('No docs available!')
 
-    @decorators.local_command()
+    @decorators.local_function()
     def get_doc(self):
         self._check_project()
         return self._base_get_doc(codeassist.get_doc)
@@ -272,11 +272,11 @@
     def auto_import(self):
         _CodeAssist(self, self.env).auto_import()
 
-    @decorators.local_command()
+    @decorators.local_function([])
     def completions(self):
         return _CodeAssist(self, self.env).completions()
 
-    @decorators.local_command()
+    @decorators.local_function([])
     def extended_completions(self):
         return _CodeAssist(self, self.env).extended_completions()
 

Re: Patches: noerror option to code_assist Anton Beloglazov 8/28/12 5:46 PM
Hi Ali,

Nice generalization! I've tested your patch, and with a couple of modifications it works fine. I've added 'return' in the local_function definition, and made get_calltip a local_function as well. Here are the updated patches:

For decorators.py:

--- decorators-backup.py 2012-07-17 10:24:30.611629650 +1000
+++ decorators.py 2012-08-29 10:39:11.867472050 +1000
+       return local_command(error_return=error_return)
 
-def global_command(key=None, prefix=False):
+def global_command(key=None, prefix=False, error_return=log_errors):
     def decorator(func):
-        func = _exception_handler(func)
+        func = _exception_handler(func, error_return)
         func.kind = 'global'
         func.prefix = prefix
         func.global_key = key


For interface.py:

--- interface-backup.py 2012-07-17 10:24:30.611629650 +1000
+++ interface.py 2012-08-29 10:42:22.736438024 +1000
@@ -157,7 +157,7 @@
     def pop_mark(self):
         self.env.pop_mark()
 
-    @decorators.local_command()
+    @decorators.local_function()
     def definition_location(self):
         definition = self._base_definition_location()
         if definition:
@@ -182,7 +182,7 @@
         self._check_project()
         self._base_show_doc(prefix, self._base_get_doc(codeassist.get_doc))
 
-    @decorators.local_command()
+    @decorators.local_function()
     def get_calltip(self):
         self._check_project()
         def _get_doc(project, text, offset, *args, **kwds):
@@ -203,7 +203,7 @@
         else:
             self.env.message('No docs available!')
 
-    @decorators.local_command()
+    @decorators.local_function()
     def get_doc(self):
         self._check_project()
         return self._base_get_doc(codeassist.get_doc)
@@ -275,11 +275,11 @@
     def auto_import(self):
         _CodeAssist(self, self.env).auto_import()
 
-    @decorators.local_command()
+    @decorators.local_function([])
     def completions(self):
         return _CodeAssist(self, self.env).completions()
 
-    @decorators.local_command()
+    @decorators.local_function([])
     def extended_completions(self):
         return _CodeAssist(self, self.env).extended_completions()

Thanks,
Anton
Re: Patches: noerror option to code_assist aligrudi 8/29/12 2:42 AM
Hi,

Anton Beloglazov <anton.be...@gmail.com> wrote:
> Nice generalization! I've tested your patch, and with a couple of
> modifications it works fine. I've added 'return' in the local_function
> definition, and made get_calltip a local_function as well. Here are the
> updated patches:
>
> For decorators.py:
>
> --- decorators-backup.py 2012-07-17 10:24:30.611629650 +1000
> +++ decorators.py 2012-08-29 10:39:11.867472050 +1000
>
> For interface.py:
>
> --- interface-backup.py 2012-07-17 10:24:30.611629650 +1000
> +++ interface.py 2012-08-29 10:42:22.736438024 +1000

I've CC'ed Anton Gritsay, the maintainer.  Anton, what do you think?
I think this patch and also my patch to make fixsyntax.py errors terser
(I've included the latter in this mail) look acceptable.

Thanks,
Ali

--- a/rope/contrib/fixsyntax.py        2012-08-27 19:42:30.516015457 +0430
+++ b/rope/contrib/fixsyntax.py        2012-08-27 19:45:00.236235582 +0430
@@ -15,7 +15,7 @@
     @utils.saveit
     def get_pymodule(self):
         """Get a `PyModule`"""
-        errors = []
+        msg = None
         code = self.code
         tries = 0
         while True:
@@ -27,19 +27,14 @@
                 return self.pycore.get_string_module(
                     code, resource=self.resource, force_errors=True)
             except exceptions.ModuleSyntaxError, e:
+                if msg is None:
+                    msg = '%s:%s %s' % (e.filename, e.lineno, e.message_)
Re: Patches: noerror option to code_assist angri 8/29/12 4:31 PM
Hi Ali, Anton,

2012/8/29 Ali Gholami Rudi <alig...@gmail.com>:
> Hi,
>
> Anton Beloglazov <anton.be...@gmail.com> wrote:
>> Nice generalization! I've tested your patch, and with a couple of
>> modifications it works fine. I've added 'return' in the local_function
>> definition, and made get_calltip a local_function as well. Here are the
>> updated patches:

Anton, could you please explain me the semantic difference between
local function and local command? It is also not clear for me why did
you use singleton object for triggering "return on error" behaviour in
_exception_handler(). Could it not be just boolean function parameter
with default to False?

And one more thing: how do I test if it works as it should? What are
steps to reproduce the problem?

>> For decorators.py:
>>
>> --- decorators-backup.py 2012-07-17 10:24:30.611629650 +1000
>> +++ decorators.py 2012-08-29 10:39:11.867472050 +1000
>>
>> For interface.py:
>>
>> --- interface-backup.py 2012-07-17 10:24:30.611629650 +1000
>> +++ interface.py 2012-08-29 10:42:22.736438024 +1000
>
> I've CC'ed Anton Gritsay, the maintainer.  Anton, what do you think?
> I think this patch and also my patch to make fixsyntax.py errors terser
> (I've included the latter in this mail) look acceptable.

I've committed this one. Thanks!

--
Anton
Re: Patches: noerror option to code_assist Anton Beloglazov 8/29/12 6:55 PM
Hi Anton,

The separation of local function and local command has been proposed by Ali in one of the previous emails. The idea is that commands are supposed to be directly executed by the user expecting to see an error message when something goes wrong. On the other hand, functions are supposed to be used programmatically inside other external commands, in which case it's more convenient to just return a special value (specified as a parameter) instead of throwing an exception. 

An example of the latter case is rope-get-calltip function in Emacs, which can be directly passed to Eldoc as eldoc-documentation-function. Throwing an exception in case of a syntax error brakes Eldoc, which just expects to get nil when there is no information available for displaying. This can be reproduced as follows:

(turn-on-eldoc-mode)
(set (make-local-variable 'eldoc-documentation-function) 'rope-get-calltip)

Then open a Python file, make a Python syntax error: currently, an exception is shown in the echo area. After applying the patch, in case of a syntax error nothing is displayed.

Another example is integration with the auto-complete mode, which I set up as follows:

;; ropemacs integration with auto-complete
(defun ac-ropemacs-candidates ()
  (mapcar (lambda (completion)
   (concat ac-prefix completion))
 (rope-completions)))

(ac-define-source nropemacs
  '((candidates . ac-ropemacs-candidates)
    (symbol     . "r")))

(ac-define-source nropemacs-dot
  '((candidates . ac-ropemacs-candidates)
    (symbol     . "r")
    (prefix     . c-dot)
    (requires   . 0)))

(add-to-list 'ac-sources 'ac-source-nropemacs)
(add-to-list 'ac-sources 'ac-source-nropemacs-dot)

The test is similar, currently when there is a Python syntax error auto-complete breaks and displays an exception in the echo area. After the patch, nothing is displayed in the echo area in case of a Python syntax error. This is desirable by the user (from my point of view), since such messages just disturb the user, while there are other special means to report syntax errors, such as flymake-mode.

Regarding the "return on error" behavior, that is the implementation proposed by Ali, I've just adopted it. Perhaps, Ali could explain his choice of a singleton object over a boolean variable. To me, it seems that a boolean parameter would work just as well.

Thanks,
Anton
Re: Patches: noerror option to code_assist aligrudi 8/30/12 7:22 AM
Hi Anton,

Anton Beloglazov <anton.be...@gmail.com> wrote:
> Regarding the "return on error" behavior, that is the implementation
> proposed by Ali, I've just adopted it. Perhaps, Ali could explain his
> choice of a singleton object over a boolean variable. To me, it seems that
> a boolean parameter would work just as well.

Actually the error_return parameter is the value to return on errors.
I added a similar parameter to local_command() mainly for implementing
local_function().  But then to retain the old behavior, I needed
to somehow tell local_command() to re-raise an exception instead of
returning a value.  I could have added a new boolean parameter like
reraise_exceptions, which, when False, forced the value of the new
error_return parameter to be returned.  This would have required adding
two new parameters to local_command(), one of which was dependent
on the other; i.e. error_return would have been meaningful only if
reraise_exceptions was False.

I decided to merge the arguments: when exceptions are caught, the value
passed as error_return should be returned, unless it is log_errors (maybe
reraise_errors is a better name).  I added the log_errors singleton
because I couldn't tell if any other value (like None) is meant to
be returned on errors or is meant to request re-raising exceptions.
I thought this is the common practice in such cases, but maybe I'm among
the few who used to use this technique (or hack, probably).

I suggest either adding comments for this new parameter and the new
global variable or using the double parameter approach.  Unless, of
course, someone has a cleaner solution.

Thanks,
Ali

Re: Patches: noerror option to code_assist Anton Beloglazov 8/30/12 5:49 PM
Hi Ali,

Thanks for the explanation! In my opinion, the two parameter approach is easier to understand since it's more explicit and doesn't require any special values.

Thanks,
Anton
Re: Patches: noerror option to code_assist aligrudi 8/31/12 2:57 AM
Hi Anton,

Anton Beloglazov <anton.be...@gmail.com> wrote:
> Thanks for the explanation! In my opinion, the two parameter approach is
> easier to understand since it's more explicit and doesn't require any
> special values.

That's OK with me and please update your patch.

Thanks,
Ali

Re: Patches: noerror option to code_assist Anton Beloglazov 8/31/12 4:24 PM
Hi Ali and Anton,

Here are the patches (only decorators.py has been updated: I've added a raise_exceptions argument):

For decorators.py:

--- decorators-backup.py 2012-08-30 11:27:46.690981338 +1000
+++ decorators.py 2012-09-01 09:18:43.820460019 +1000
@@ -47,13 +47,15 @@
                     exceptions.ModuleSyntaxError,
                     exceptions.BadIdentifierError)
 
-def _exception_handler(func):
+def _exception_handler(func, raise_exceptions, error_return):
     def newfunc(*args, **kwds):
         try:
             return func(*args, **kwds)
         except exceptions.RopeError, e:
             short = None
             if isinstance(e, input_exceptions):
+                if not raise_exceptions:
+                    return error_return
                 short = _exception_message(e)
             logger(str(traceback.format_exc()), short)
     newfunc.__name__ = func.__name__
@@ -72,10 +74,12 @@
         return func
     return decorator
 
-
-def local_command(key=None, prefix=False, shortcut=None, name=None):
+def local_command(key=None, prefix=False, shortcut=None,
+                  name=None, raise_exceptions=True,
+                  error_return=None):
     def decorator(func, name=name):
-        func = _exception_handler(func)
+        func = _exception_handler(func, raise_exceptions,
+                                  error_return)
         func.kind = 'local'
         func.prefix = prefix
         func.local_key = key
@@ -86,10 +90,15 @@
         return func
     return decorator
 
+def local_function(error_return=None):
+       return local_command(raise_exceptions=False,
+                            error_return=error_return)
 
-def global_command(key=None, prefix=False):
+def global_command(key=None, prefix=False, raise_exceptions=True,
+                   error_return=None):
     def decorator(func):
-        func = _exception_handler(func)
+        func = _exception_handler(func, raise_exceptions,
+                                  error_return)
         func.kind = 'global'
         func.prefix = prefix
         func.global_key = key

For interface.py

--- interface-backup.py 2012-07-17 10:24:30.611629650 +1000
+++ interface.py 2012-08-29 10:42:22.736438024 +1000

Thanks,
Anton
Re: Patches: noerror option to code_assist Anton Beloglazov 8/31/12 4:25 PM
A side note: I'll be away for the next two weeks. If there are any issues, I'll only be able to look into them when I come back.

Best regards,
Anton
Re: Patches: noerror option to code_assist Anton Beloglazov 9/12/12 6:41 PM
Hi Ali and Anton,

Is there any problem with accepting the patches?

Thanks,
Anton
Re: Patches: noerror option to code_assist angri 9/12/12 10:38 PM
Hi Anton, hi Ali,

I'm sorry for the delay. I'm very short on free time these days, but
I'll definitely take care of the patches until the middle of next
week. Thanks!

--
Anton

2012/9/13 Anton Beloglazov <anton.be...@gmail.com>:
Re: Patches: noerror option to code_assist angri 9/17/12 12:46 PM
The patch is now in master. Please test and let me know if something
is not quite right.

Thanks, Anton and Ali!

--
Anton

2012/9/13  <an...@angri.ru>:
Re: Patches: noerror option to code_assist Anton Beloglazov 9/17/12 4:42 PM
I've just tested it, it works. Thanks, Anton and Ali!

Best regards,
Anton
More topics »