Swig wrapper for apertium-tagger

[singh-lokendra]

The entire command is passed to the apertium_tagger constructor

• Set optind=1 in the constructor to ensure multiple instances of apertium_tagger can be created.

[sushain97]

Was the rename necessary? It's kind of annoying that Git isn't recognizing the rename. Any thoughts on that? Moreover it seems like we're going against the convention now since the apertium-foo.cc file by convention seems to contain main whereas the foo.cc file contains the actual implementation.

[singh-lokendra]

Was the rename necessary?

I though it would be better to have the tagger.cc declared in tagger.h instead of apertium_tagger.h

It's kind of annoying that Git isn't recognizing the rename. Any thoughts on that?

Its probably showing all the changes, and doesn't recognize renaming as a part of it, might be due to editing the header gaurds after renaming the file. It do recognize file rename 52a32a5.

Moreover it seems like we're going against the convention now since the apertium-foo.cc file by convention seems to contain main whereas the foo.cc file contains the actual implementation.

I've kept the main in apertium_tagger.cc and moved the class definition to tagger.cc. Can you elaborate your concern.

[sushain97]

Ah, okay. That all makes sense.

Why were the headers moved from the .cc to the .h file? i.e. all this stuff: https://github.com/apertium/apertium/pull/52/files?file-filters%5B%5D=.cc&file-filters%5B%5D=.h&file-filters%5B%5D=.i&file-filters%5B%5D=.in#diff-d30673bddfe5dab2e86c97f0ca3b7351R21.

Also, is it necessary that all these source files are included? i.e. https://github.com/apertium/apertium/pull/52/files?file-filters%5B%5D=.cc&file-filters%5B%5D=.h&file-filters%5B%5D=.i&file-filters%5B%5D=.in#diff-9e56523df55a28867873fdee4107aebbR22. e.g. what is a.cc?

[singh-lokendra]

Why were the headers moved from the .cc to the .h file?

Since I've added these files in tagger.h, I didn't add them again in tagger.cc. Should I add them again in .cc file

Also, is it necessary that all these source files are included?

Yes, I had to lookup all the undefined objects in shared object and I included all the required files.

e.g. what is a.cc?

Its required for operator overloading. Also a.h is included in various files. Hence its .cc file needs to be linked as well.

[sushain97]

Since I've added these files in tagger.h, I didn't add them again in tagger.cc. Should I add them again in .cc file

Standard convention is to add includes in the .cc file when possible and only in the header when types from them are necessary. Can they be moved back to their original location?

Yes, I had to lookup all the undefined objects in shared object and I included all the required files.

I see. This wasn't necessary before (for any of the other wrappers, that is)? Does it have something to do with you moving the includes?

Its required for operator overloading. Also a.h is included in various files. Hence its .cc file needs to be linked as well.

Ah. Odd name...

[singh-lokendra]

Standard convention is to add includes in the .cc file when possible and only in the header when types from them are necessary. Can they be moved back to their original location?

I'll check their dependencies on swig and move them back to the .cc file. Should I fix this as well https://github.com/apertium/apertium/blob/master/apertium/pretransfer.cc#L1

I see. This wasn't necessary before (for any of the other wrappers, that is)? Does it have something to do with you moving the includes?

These are dependencies for tagger.cc.

[sushain97]

I'll check their dependencies on swig and move them back to the .cc file. Should I fix this as well https://github.com/apertium/apertium/blob/master/apertium/pretransfer.cc#L1

Ah, yes. That would be ideal. My bad for not noticing the issue earlier.

These are dependencies for tagger.cc.

Hmm, I see. Maybe we should separate the dependencies in the Python file with comments?

[sushain97]

Some notes. Mostly nits.

[sushain97]

@Vaydheesh can you bring changes from master into this branch? Also perhaps update the Travis build to actually build the Python bindings?

[singh-lokendra]

@Vaydheesh can you bring changes from master into this branch? Also perhaps update the Travis build to actually build the Python bindings?

Are these commands the required for updating my branch? Where upstream points to apertium-master and origin is my fork.

git pull upstream master
git push master origin
git checkout swig_wrapper_tagger
git rebase master
git push origin swig_wrapper_tagger --force

[sushain97]

Are these commands the required for updating my branch? Where upstream points to apertium-master and origin is my fork.

It looks right. There should be lots of instructions on the web re. updating a fork's branch.

[singh-lokendra]

Also perhaps update the Travis build to actually build the Python bindings?

Should I also add test in makefile to support make test https://github.com/apertium/apertium/blob/master/.travis.yml#L13?

[sushain97]

Should I also add test in makefile to support make test https://github.com/apertium/apertium/blob/master/.travis.yml#L13?

Huh? The test target already exists: https://github.com/apertium/apertium/blob/master/Makefile.am#L25 ?

[singh-lokendra]

I don't think that it would check for importing the wrapper. python -m apertium_core would fail if all the required symbols aren't present in the shared object https://github.com/Vaydheesh/apertium/blob/swig_wrapper_tagger/python/setup.py.in#L22

[sushain97]

Oh, are you suggesting adding more tests? If so, go ahead!

[sushain97]

Oh wow. I didn't realize this many functions were const-qualified. I'm not super comfortable with this change without some other input. Another, albeit hacky, approach is adding a patch file that gets applied before building the SWIG wrapper. @unhammer / @TinoDidriksen any thoughts? The diff in question is 51e7c28 and the associated discussion is here: apertium/apertium-python#40 (comment).

[singh-lokendra]

I have added the script in TESTS 1a177a2#diff-ad9a2d1628d58878eff1ecc2e77834d8R5 still it isn't executed https://travis-ci.com/apertium/apertium/jobs/215825709#L1739. How can I fix this?

[sushain97]

Don't you need to execute make test in the subfolder as well? Not too familiar with autotools but I can look more into it if you're unable to figure it out. Regardless, I don't consider it a blocker. More concerned with the diff that removes const qualifiers.

[singh-lokendra]

I tried but can't use the python import, so I've reverted back to previous state

[sushain97]

I tried but can't use the python import, so I've reverted back to previous state

You could probably tackle this by forcing the distro Python version: https://docs.travis-ci.com/user/languages/python/#travis-ci-uses-isolated-virtualenvs. Not too worried about it though.

[sushain97]

This and others don't seem necessary?

[singh-lokendra]

I started with basic_tagger.h, and made changes according to the errors reported
Error without the changes stream_5_3_3_tagger_trainer.cc:30:20: error: binding reference of type ‘Apertium::basic_Tagger::Flags&’ to ‘const Apertium::basic_Tagger::Flags’ discards qualifiers