-
Notifications
You must be signed in to change notification settings - Fork 106
Conversation
@peerdavid 👋 thank you for the PR! I don't have much time this week to look at this but my proposal would be something along these lines:
What do you think? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank so much for doing this!
I added a few minor comments.
I would also like to change the README.md
to be more explicit about this being optional. As I said in the issue, I don't want to discourage non-power users by letting them think they need to install a lot of dependencies. I'm going to propose some changes to your PR in a separate one to see if you are cool with them.
Thank you!
shell/geta.go
Outdated
if err != nil { | ||
// If we could not parse the time correctly, we still continue | ||
// with the execution such that the pdf is downloaded... | ||
fmt.Println(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please use the logging facilities instead of standard output/err?
Something like:
log.Trace.Println(err)
or in this case, if this is just a warning:
log.Warning.Println(err)
In general, we should try to use the log facilities instead of fmt.Print
as much as we can in this file. Either that or use the ishell
context to print relevant stuff.
shell/geta.go
Outdated
// If we could not parse the time correctly, we still continue | ||
// with the execution such that the pdf is downloaded... | ||
fmt.Println(err) | ||
fmt.Println("(Warning) Could not parse modified time. Overwrite existing file.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
shell/geta.go
Outdated
if !os.IsNotExist(err) { | ||
outputFileModTime := outputFile.ModTime() | ||
if(outputFileModTime.Equal(modifiedClientTime)){ | ||
fmt.Println("Nothing changed since last download. Skip. ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. This looks like log.Trace
candidate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments => fixed all invalid log statements
shell/geta.go
Outdated
|
||
// Convert to pdf | ||
exportPdf := os.Getenv("GOPATH") + "/src/github.com/juruen/rmapi/tools/exportAnnotatedPdf" | ||
rM2svg := os.Getenv("GOPATH") + "/src/github.com/juruen/rmapi/tools/rM2svg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we create a file/function where we encapsulate checks to verify if the required tools are installed. We can then use it in https://github.com/juruen/rmapi/blob/master/shell/shell.go#L54 to optinally add these new commands to the shell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I will fix a bug that I found right now and afterwards I will start with this verification
…re disabled 2. Updated README.md to be more explicit about the optional commands geta and mgeta
README.md
Outdated
|
||
|
||
# Thanks to | ||
[1] rmapi, https://github.com/juruen/rmapi <br /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can drop this line :)
Does this support the latest remarkable line format? The export notebook can be made platform agnostic as done here: reHackable/maxio#22 (and probably it could be ported to go in a similar fashion) |
Currently not but I will update to the latest line format (in the fork that I created) when my RM device is also running on version 1.7 |
Hello @mseri, This normally temporary feature (as it should be shifted to a full go implementation someday) relies as you said on the Effectively, it seems to be working with the previous Apparently, there is an open issue on the project here. So I think the implementation has to be made there. However, we started slowly the shifting to a pure go implementation. We created and merged recently the package See https://github.com/juruen/rmapi/blob/master/encoding/rm/rm.go#L43. I am not sure whether it makes sense to continue supporting the previous format as well. @peerdavid, just seen you reply and did not know that you started a fork with the support of the new format! Good news! |
@lobre there is already a version working with the new format (see the comments here reHackable/maxio#22 (comment) and here reHackable/maxio#27 (comment), the last version of the script is http://www.ucl.ac.uk/~ucecesf/tmp/rm2svg.py). The https://github.com/reHackable/maxio repo seems unmaintained, there are pending PRs to get rid of external dependencies and other improvements that are ignored since months. I don't think that supporting the old version makes sense, the recent updates are more stable, more ergonomic and more flexible, I hope people are uploading their remarkables. |
@peerdavid |
Sry, my initial implementation which uses a python script etc. is no more maintained - they implemented in my opinion a much better golang native version of mgeta which should also be in the masterbranch. |
This allows to execute geta (read annotated file) and mgeta (read multiple annotated files)
Note: With this PR we introduce additional dependencies (the transformation is executed i.e. in a bash script). Those are listed in README.md
ToDo's