Skip to content
This repository has been archived by the owner on Jul 2, 2024. It is now read-only.

Lobre/lines format #52

Merged
merged 12 commits into from
Feb 18, 2019
Merged

Lobre/lines format #52

merged 12 commits into from
Feb 18, 2019

Conversation

lobre
Copy link
Collaborator

@lobre lobre commented Feb 13, 2019

Hello @juruen,

I have started a refactoring with this pull request by creating 2 separate packages for 2 well defined purposes:

  • archive: for interacting with a remarkable ".zip" file more easily.
  • encoding/rm: for marshaling/unmarshaling a ".rm" file.

This would help building a solid base around remarkable formats before starting to analyse the new SVG feature (#41).

The package archive is more or less a wrapper around the standard "archive/zip" package and the interface is similar.

I have added the package "rm" into an "encoding" folder as made in the standard library for different formats (e.g. encoding/json). That would allow for instance to create other encoders/decoders for other remarkable files present in the ".zip" (e.g. encoding/pagedata). The code of this package is by the way a lot inspired by what you did in "annotations/rm.go". I have added the "encoding/rm/marshal.go:MarshalBinary" method as TODO so far because did not take the time to implement it.

I have refactored your annotations package to use these 2 new introduced packages. But I did not take time to refactor the "api" package but this would be nice to use them as well. As far as I understand, the "util/zipdoc.go" could disappear if everything is refactored.

I let you have a look and come back to me if you see any problems with this implementation!
Thanks,
Loric

@juruen
Copy link
Owner

juruen commented Feb 16, 2019

Hey @lobre,

This looks great! Planning to take a look and merge it this weekend!

Copy link
Owner

@juruen juruen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice job! Looks super clean and lays the foundations to iterate over it! 👏 👏 👏

I just added one comment but am approving the PR already!

Great work!

archive/reader.go Outdated Show resolved Hide resolved
@juruen juruen merged commit 6e91f80 into juruen:master Feb 18, 2019
@lobre lobre deleted the lobre/lines-format branch September 6, 2019 06:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants