Async method not processed - wrong generated #11
Cerrada
abierta hace 5 años por deloptes
·
10 comentarios
Cargando…
Referencia en una nueva incidencia
Aún no existe contenido.
Eliminar la rama con el nombre '%!s(<nil>)'
Eliminar una rama es permanente. NO PUEDE deshacerse. ¿Continuar?
Basic information
Description
when method is annotated as Async, the generated code does not work because handling is wrong
Steps to reproduce
It should be
see commit
030ba0231b
in pull #8After building the code and testing I found out that now if we call dbus Introspect, it will return the xml with the async method generated twice
see commit
a0cf1c4793
in pull #8, however I think the approach is wrong. it should generate the method with annotation. However in this case it is hard to tell which method we want to use as visible in this issue.Perhaps we should prevent generation of non async method with same name, because this method is marked as async - this will require quite of a code change I think.
There is interesting discussion around this.
So in general to add "Async" is wrong because "TestAsync" is not in the introspection. To me it looks like in case a method is annotated as async - the blocking method should not be handled at all and vice versa.
This will require more brain power.
I opened a new pull request #12 - I would remove those commits from here, but I wait for you to comment.
I do not understand why you introduce a new property "method.haveAsync". IMO "method.async" is sufficient in this case. As you and Michele are the main drivers, I would suggest he has to look into it, I am happy when it works like it should. I will test in the evening (hopefully), what code is exactly generated and how useful it is.
My intention is simple – not to make the code unnecessarily complex.
At the moment of introspection generation, you have only the current method information available. When you see that
(*it).async
isfalse
=> introspection will be generated, but you don't know if there is the same method in the list as async. You would have to search the list to see if you can find that async method. In my proposal, the information about the existence of the async method is kept at the beginning – the sync method has the set flag(*it).haveAsync
totrue
. Information about the existence of the async method is therefore known immediately – simple.Of course, you can ignore my commit if you have a simpler solution 😸
I will take a look sooner or later and let you know my view as well. I prefer to go one step at a time in the correct sequence. Yesterday I started looking at #7 and immediately ran into the problem described in #17. So IMO next step is to fix #17, then we can go back to #7 and #11.
The solution for this issue seems to be simple and independent of the more extensive changes that are in other issues. Therefore, I thought it would be possible to complete and close it. This will help to make fewer pending changes. Just my two cents.
OK, thanks Slavek, I understand now. I was thinking checking if size is > 1 is enough. Let us see what Michele will do. Thank you!
Fixed by PR #12.