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

Backup Diff: Adding show-diff command (WIP) #196

Merged
merged 2 commits into from Aug 20, 2018

Conversation

Projects
None yet
3 participants
@matheussbernardo
Contributor

matheussbernardo commented Jun 10, 2018

Hello @Ana06 @cornelius

On this PR that is a WIP I used json-diff to diff the backup
from the online version. To do that I created a command on Cli.rb and created a method on Backup.rb to show the backup as json.

Is this what you were expecting or a custom algorithm of diffing?

After the diff the operations array I need to parse and generate a set of requests to the trello api to do the restore command. Maybe the Diff should be more user friendly and then only use a private version of the diff to the restore command.

Appreciate any suggestions and improvements.

@Ana06

@matheussbernardo

On this PR that is a WIP I used json-diff to diff the backup
from the online version. To do that I created a command on Cli.rb and created a method on Backup.rb to show the backup as json.

Is this what you were expecting or a custom algorithm of diffing?

I don't think we need a custom diff. At least if there is not a reason for it, we can use something already done. Have you considered using easy-diff? It seems to be more used than json-diff, although a little bit older. It wouldn't be a big deal to change of gem or even providing a custom implementation if it doens't work at any point, but maybe worthwhile to give it a though.

After the diff the operations array I need to parse and generate a set of requests to the trello api to do the restore command. Maybe the Diff should be more user friendly and then only use a private version of the diff to the restore command.

I am not sure of what you exactly mean. 😕

lib/cli.rb Outdated
@@ -206,7 +206,7 @@ def backup
Backup.new(@@settings).backup(board_id(options['board-id']))
end

This comment has been minimized.

@Ana06

Ana06 Jun 11, 2018

Member

rubocop doesn't like this... 😭

@@ -25,6 +25,11 @@ def backup(board_id)
def list
Dir.entries(@directory).reject { |d| d =~ /^\./ }
end
def show_json(board_id)
backup_path = File.join(@directory, board_id)

This comment has been minimized.

@Ana06

Ana06 Jun 11, 2018

Member

File#join accept more than 2 parameters, so you could directly do:

backup_path = File.join(@directory, board_id, 'board.json')
board = JSON.parse(File.read(backup_path))
def show_json(board_id)
backup_path = File.join(@directory, board_id)
board = JSON.parse(File.read(File.join(backup_path, 'board.json')))

This comment has been minimized.

@Ana06

Ana06 Jun 11, 2018

Member

you can remove the board variable 😉

@@ -25,6 +25,11 @@ def backup(board_id)
def list
Dir.entries(@directory).reject { |d| d =~ /^\./ }
end
def show_json(board_id)

This comment has been minimized.

@Ana06

Ana06 Jun 11, 2018

Member

what about renaming show_json to show_diff(local_board_id, online_board_id) and move here the put you have in cli? I think that code is really related to the backup and should be in the Backup model 😄

This comment has been minimized.

@Ana06

Ana06 Jun 11, 2018

Member

I also wonder if you should do something like what we have in the Backup#show command:

out = options[:output] || STDOUT
@Ana06

This comment has been minimized.

Member

Ana06 commented Jun 11, 2018

@cornelius

This comment has been minimized.

Member

cornelius commented Jun 11, 2018

How does the diff look like?

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Jun 11, 2018

{"op"=>"add", "path"=>"/cards/0", "value"=>{"id"=>"5b1c93361892bfbcc313f138", "checkItemStates"=>[], "closed"=>false, "dateLastActivity"=>"2018-06-10T02:55:50.899Z", "desc"=>"", "descData"=>nil, "idBoard"=>"5b15a5f0ae99fe88727fbb1e", "idList"=>"5b15a5f143c9fcbe16c9a317", "idMembersVoted"=>[], "idShort"=>1, "idAttachmentCover"=>nil, "idLabels"=>[], "manualCoverAttachment"=>false, "name"=>"(6) Task 1", "pos"=>65535, "shortLink"=>"tOzVwgmw", "badges"=>{"votes"=>0, "attachmentsByType"=>{"trello"=>{"board"=>0, "card"=>0}}, "viewingMemberVoted"=>false, "subscribed"=>false, "fogbugz"=>"", "checkItems"=>0, "checkItemsChecked"=>0, "comments"=>0, "attachments"=>0, "description"=>false, "due"=>nil, "dueComplete"=>false}, "dueComplete"=>false, "due"=>nil, "idChecklists"=>[], "idMembers"=>[], "labels"=>[], "shortUrl"=>"https://trello.com/c/tOzVwgmw", "subscribed"=>false, "url"=>"https://trello.com/c/tOzVwgmw/1-6-task-1", "checklists"=>[]}}

It is really ugly right now, so this example is an operation of type add, in this example the online version is different from the bakcup because the online has a card.

@Ana06

@matheussbernardo you can try to use ruby pretty print (pp) and you will get something like:

{"op"=>"add",
 "path"=>"/cards/0",
 "value"=>
  {"id"=>"5b1c93361892bfbcc313f138",
   "checkItemStates"=>[],
   "closed"=>false,
   "dateLastActivity"=>"2018-06-10T02:55:50.899Z",
   "desc"=>"",
   "descData"=>nil,
   "idBoard"=>"5b15a5f0ae99fe88727fbb1e",
   "idList"=>"5b15a5f143c9fcbe16c9a317",
   "idMembersVoted"=>[],
   "idShort"=>1,
   "idAttachmentCover"=>nil,
   "idLabels"=>[],
   "manualCoverAttachment"=>false,
   "name"=>"(6) Task 1",
   "pos"=>65535,
   "shortLink"=>"tOzVwgmw",
   "badges"=>
    {"votes"=>0,
     "attachmentsByType"=>{"trello"=>{"board"=>0, "card"=>0}},
     "viewingMemberVoted"=>false,
     "subscribed"=>false,
     "fogbugz"=>"",
     "checkItems"=>0,
     "checkItemsChecked"=>0,
     "comments"=>0,
     "attachments"=>0,
     "description"=>false,
     "due"=>nil,
     "dueComplete"=>false},
   "dueComplete"=>false,
   "due"=>nil,
   "idChecklists"=>[],
   "idMembers"=>[],
   "labels"=>[],
   "shortUrl"=>"https://trello.com/c/tOzVwgmw",
   "subscribed"=>false,
   "url"=>"https://trello.com/c/tOzVwgmw/1-6-task-1",
   "checklists"=>[]}}

Another option would be using awesome_print.

And the last option is to parser the result to customize the output. But I would avoid it if it is not strictly necessary, so that this keep working if the result from Trello changes at some point. And we can also keep it simple by using pp and complicate it later on. 🤔

@cornelius

This comment has been minimized.

Member

cornelius commented Jun 21, 2018

I'm all for complicating it later on ;-) Let's start with something basic like pretty printing the json-diff results.

@Ana06

Ana06 requested changes Jul 2, 2018 edited

@matheussbernardo LGTM, can you please squash your commits and add a nice commit description to that we can merge it? 🙏

@Ana06

This comment has been minimized.

Member

Ana06 commented Jul 2, 2018

and rebase! 😉

JsonDiff.diff(board_backup, board_online)
end
def show(board_id, version,options = {})

This comment has been minimized.

@Ana06

Ana06 Jul 3, 2018

Member

you introduced here an offense... Rubocop is complaining 😭

@matheussbernardo matheussbernardo force-pushed the matheussbernardo:backup_diff branch 6 times, most recently from 5ae8b0e to afb67c7 Jul 3, 2018

@matheussbernardo matheussbernardo force-pushed the matheussbernardo:backup_diff branch from afb67c7 to 2111dff Jul 13, 2018

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Jul 24, 2018

@Ana06 I dont know why the travis broken! it says something about sudo? I did not changed any .travis.yml config

@Ana06

@matheussbernardo

@Ana06 I dont know why the travis broken! it says something about sudo? I did not changed any .travis.yml config

Some times Travis get stuck and it is needed to restart it. Probably when you pushed need changes it was restarted and that is why it works now. Can you please rebase so that we can get this merged? 😄

@Ana06

This comment has been minimized.

Member

Ana06 commented Aug 2, 2018

@matheussbernardo ping! 🙄

@matheussbernardo matheussbernardo force-pushed the matheussbernardo:backup_diff branch 2 times, most recently from 6598ad6 to 2469e92 Aug 3, 2018

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Aug 5, 2018

@Ana06 ready to merge

@Ana06

the code LGTM, but the commit messages can be improved. Please limit the commit title of the first commit to 50 characters and add a description which explains the changes in the second. Read https://chris.beams.io/posts/git-commit for more details.

@matheussbernardo matheussbernardo force-pushed the matheussbernardo:backup_diff branch from 2469e92 to f4ab184 Aug 11, 2018

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Aug 11, 2018

is it better now? @Ana06

@Ana06

@matheussbernardo

is it better now? @Ana06

great! 💘 you need to rebase thought to ensure we don't break anything:

This branch is out-of-date with the base branch

matheussbernardo added some commits Jun 10, 2018

Add show-diff command and more than one local backups
* Creates show-diff command to show diff between online and local backup of the board

* Implements multiple version of local backup

* Implements diff between local versions

* Improves list-backups to show all backups version
Add option board version to command backup show
* Create an option on the command backup show to select the version of the backup to be used by the command

@matheussbernardo matheussbernardo force-pushed the matheussbernardo:backup_diff branch from f4ab184 to 9bf2def Aug 12, 2018

@matheussbernardo

This comment has been minimized.

Contributor

matheussbernardo commented Aug 12, 2018

@Ana06 rebased!

@Ana06

Ana06 approved these changes Aug 20, 2018

great @matheussbernardo! Thanks 😉

@Ana06 Ana06 merged commit 76f8eaf into openSUSE:master Aug 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment