Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generates OSGi bundle and source bundle. #6

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

aborg0
Copy link
Contributor

@aborg0 aborg0 commented Jun 26, 2016

This change adds OSGi support to parso and bumps the version number to 2.1.0-SNAPSHOT.

@aborg0
Copy link
Contributor Author

aborg0 commented Jun 26, 2016

In another branch (aborg0/time_handling), I have merged some improvements from the KNIME's version of parso. In case you like them, I would like to ask your opinion about another change, which I could also merge to that branch: tracking the line number and improved error reporting based on those (feel free to check the KNIME parso lib, look for the ParsoException and the byteArrayWithData method).

@srozsnyai
Copy link

@aborg0 I'm wondering if your improvements fix the issues with DateTime formats. See my pull request with a fix for a specific time format issue I ran into.

@srozsnyai
Copy link

@aborg0 Never mind ... your fork surfaces the bufferunderflow exception as well...

@aborg0
Copy link
Contributor Author

aborg0 commented Jun 28, 2016

@srozsnyai You are right, the time-related patches are not containing your proposed fix as I have no SAS7BDAT file which I could use to reproduce/fix, KNIME customers have not yet reported a problem about that.

Fixing .gitignore to better support eclipse development.
@printsev
Copy link
Contributor

printsev commented Jul 6, 2016

Honestly I'm a little bit reluctant to make these type of changes... From my perspective parso should be a simple jar, and if an osgi bundle is required, either a separate project (e.g. parso-osgi-bundle) should be created or the parso jar should be converted to osgi bundle using one of the standard approaches.

Please let me know what you think so we try to satisfy everyone :)

@aborg0
Copy link
Contributor Author

aborg0 commented Jul 6, 2016

An OSGi bundle is just a simple jar -you can check it if you try the usual packaging on this branch- with some extra version information (the only* difference would be that on maven central repository not "jar" would be the text on the download button, but "bundle", those who are not familiar with it, would hardly recognize). Using gradle as I used is the standard way to generate an OSGi bundle. ;) (Though I have to admit most projects do not care to make the source jar a bundle too. If that is too much, I can remove that.)

The reason why I think this should be a bundle and not a separate project: when epam signs the jar, the bundle should be also signed, while if the owner of the OSGi project have no access to the key used for signing, another should be used, so authenticity is lost.

*: There is one more thing, which might be a downside: semantic versioning should be used for releases, so in case there are breaking changes in the public interface -I doubt there will be many-, the version number have to be incremented.

Michael George and others added 2 commits July 20, 2016 13:48
Add support for controlByte 0x50 (i.e. 17 or more @ symbols) within decompressRow().

Control byte 0x50 is essentially the same as controlByte 0x60 and 0x70 but for the @ symbol (0x40) rather than space (0x20) or NUL (0x00).

This type of RLE was encountered in my SAS dataset and I have confirmed that repeated @ characters are encoded using control byte 0x50.
Add support for controlByte 0x50 within decompressRow()
@aborg0
Copy link
Contributor Author

aborg0 commented Sep 3, 2016

@printsev would this change be more acceptable if the implementation package was not exported? I think that can be done with OSGi services (probably a nicer solution). Maybe you require unit tests for OSGi functionality too?
Obviously a choice of not supporting OSGi is also acceptable, we can generate bundles from released parso builds quite easily (either with maven or gradle), so it is not a problem for us, just thought others might benefit from an official OSGi bundle too (which would still allow using parso as a regular jar too).

Have you had a chance to take a look at the other changes in the time_handling branch? Do you think that would be useful for parso, or we should move them to a separate processing step? (I think without the time handling changes the encoding and the improved error handling (which is not yet added, just a precursor of it - a new exception type should contain the row number too) might be useful.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants