Cleanup code and change to a more pythonic API #6
Conversation
This method is buggy and I don't see any use case where a user would like to change those values from python code. This method is buggy cause : - Assigning string python object to 'char *' C variable do not copy but directly use the inner data array of the string. Once the string python object is garbage collected, the 'char *' points to garbage data. - Assigning to the fields items is buggy cause the for loop set only the first item of the list (and after having set it to NULL) This method has never work and so probably no one use it. There will be no problem if we remove it.
The fields struct is already initialized by C code before parsing a tag line (in the tags file) and setting the tagEntry. We do not need to do it ourselves.
Most 'char *' attributes, function arguments or return values are const. Keep it in Cython. In __getitem__, result should be 'const char*' now, but Cython is smart enough to infer it itself. So no need to be explicit.
Behave more link a dict, raise a KeyError instead of returning None.
Use collections.abc.Mapping to let TagEntry be usable has a Mapping. cdef class cannot inherit from a Python class. So rename it to cTagEntry and use classic class TagEntry to do the Mixin. # Conflicts: # README.rst
The common Python idiom is to raise Exception instead of returning status values. Move code of open method in __cinit__. No more access to CTags attributes 'opened' and 'error_number'. This is no more needed with the raise of OSError. # Conflicts: # README.rst
Taking a argument as ouput is a C idiom. In python, we return the value. The entry is a plain dict, not anymore a class with Mapping API. More than the pythonic API, this fix also a bug : The C tagEntry share the same internal buffer to store the datas. ctags* function "just" changes the entries' pointers to the right place in the buffer. Using one of those functions will invalidate any other entries already returned. By using a dict we make a plain copy of the string and so entry are now independent of the internal state of the C code.
We do not use first/next/find* methods anymore. '*next' methods are depends of the context set by other methods. Hiding those methods in a high level API ensure that they are not badly used. 'first' and 'find' can respectively replace by next(ctagsfile.all_tags()) and next(ctagsfile.find_tags(...))
| if not self.info.status.opened: | ||
| raise Exception('Invalid tag file') | ||
| else: | ||
| if key == 'author': |
There was a problem hiding this comment.
I'd do something like this to further clean up the code
if key in ('author', 'name', 'url', 'version'):
ret = getattr(self.info.program, key)There was a problem hiding this comment.
I'm not sure getattr works for c structure. Maybe cython is smart enough.
I will test.
There was a problem hiding this comment.
Well, Cython is not smart enough :) getattr doesn't work on c structure.
|
That looks like a nice improvement. |
|
I could be done for the path of the ctags file (using filesystem encoding). However, for the tags entries it seems a bit more complicated : Which encoding use ? |
|
How about this: add an encoding parameter when loading the tag file that defaults to UTF8 and show people how to specify an encoding when generating the ctags file in the readme. |
…tent. Encoding argument to CTags file is used to automatically encode/decode content of the CTags file.
|
Last commit add the encoding/encoding_errors parameter at CTags creation. |
|
Could you please move the unrelated ckeanups like in the readme to another pull request? Otherwise it's just too much to review at once. Thanks |
|
What's the state of this? |
|
@mgautierfr Hello, same question as Jonas, what's the state of this? |
I think it is better to give a new name like python3-readtags, python-readtags, python3-libreadtags, or python3-libreadtags to the project. |
|
@mgautierfr can we take over this pull request? |
|
Yes, let's do it :) |
|
Sorry my no response to first ping. Yes, take over this PR. I don't use |
Big code clean, fix bugs and change API to a more pythonic one (and less prone to error).
This rewrite almost all the pyx file :)
This cause a API break, so it should be preferable to release a new major version.