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

Add basic export and import commands #75

Merged
merged 17 commits into from Oct 14, 2019

Conversation

dorcastan
Copy link

@dorcastan dorcastan commented Oct 9, 2019

Resolves #33 and resolves #36.

  • Bookmarks can be exported to or imported from the folder data/bookmarks.
  • For now, export applies to all of Mark's data, while import only imports bookmarks.
  • import is an undoable command (Mark's state is saved).

In preparation for implementing export/ import commands.
Create StorageStub in CommandUtil to maintain existing JUnit tests.
Export all of Mark's data into a given file in /data/bookmarks/.
First increment of the export command.
Replace all of Mark's bookmarks with bookmarks from a given file.
Allow a Storage to be specified when asserting command success or
command failure using the CommandTestUtil methods.
Avoid throwing raw exception type, and use requireAllNonNull to reduce
number of statements.
@Na-Nazhou Na-Nazhou added this to Review in progress in v1.2 Oct 10, 2019
Modify import functionality: add imported bookmarks to Mark instead of
replacing existing bookmarks. Duplicate bookmarks are skipped and
shown in the command result.
Add logging for ExportCommand and improve ImportCommand log messages.
Append the file extension (.json) to the given filename when parsing
export and import commands.
Replace usage of Paths#get(...) with Path#of(...).
Remove unused import statements and update parser tests to include
file extension.
To Do: Abstract the conversion of a filename to a Path into a separate
method.
@Na-Nazhou Na-Nazhou self-requested a review October 11, 2019 08:04
@dorcastan dorcastan changed the title Add basic export and import commands WIP: Add basic export and import commands Oct 11, 2019
Copy link

@openorclose openorclose left a comment

Choose a reason for hiding this comment

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

wow your javadocs are legit

ok ya it's quite strange to see so much logic going on in the commands, but then it doesn't make sense to do it in the Model as well, since that's more for updating stuff related to Mark.

Maybe you can put importMarkFrom and exportMarkTo in the Storage interface, and then call methods from there instead?

So Model handles all the mark related stuff, and Storage handles all the file io stuff

src/main/java/seedu/mark/logic/commands/ImportCommand.java Outdated Show resolved Hide resolved
src/main/java/seedu/mark/logic/commands/ImportCommand.java Outdated Show resolved Hide resolved
src/main/java/seedu/mark/logic/commands/ImportCommand.java Outdated Show resolved Hide resolved
* Adds bookmarks from the given list of bookmarks to the model and adds
* duplicate bookmarks to the list of skipped bookmarks.
*/
private void addBookmarksToModel(Model model, ObservableList<Bookmark> bookmarksToAdd,

Choose a reason for hiding this comment

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

I'm starting to have a feeling all these methods should be somewhere in the model, doesn't feel right to have so much logic going on here.

Copy link
Author

Choose a reason for hiding this comment

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

Right... I'll go figure out where to put it. And likewise for readMarkFromStorageFile (moving to Storage).

Modify ImportCommandTest to account for folder structure not being
imported yet. Modify AddFolderCommandTest to account for additional
storage parameter in Command#execute().

Bug to fix: Folder structure is ignored during import
Resolve conflicts between Redo/Undo and Import/Export.
Move StorageStub from CommandTestUtil to seedu.mark.storage in test
module, to match ModelStub.
Copy link

@TSAI-HSIAO-HAN TSAI-HSIAO-HAN left a comment

Choose a reason for hiding this comment

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

No big problem. Just read the suggestion.

@dorcastan dorcastan added this to the v1.2 milestone Oct 12, 2019
Move the "add bookmarks/folders to the model" logic into a separate
class for better OOP. Modify Model to handle the addition of multiple
bookmarks or folders.

To-Do: implement folder addition.
Fix equals() method and add a JUnit test for it.
MarkImporter(Model model, ReadOnlyMark markToImport) {
this.model = model;
this.foldersToImport = markToImport.getFolderStructure();
for (Bookmark bookmark : markToImport.getBookmarkList()) {
Copy link
Author

Choose a reason for hiding this comment

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

Executing logic in the constructor seems a bit weird, but I'm not sure how to fix it without making ImportCommand call another MarkImporter method after instantiating it.

Update signature of GotoCommand and TabCommand.
And update variable name in ImportCommand#equals()
@dorcastan dorcastan changed the title WIP: Add basic export and import commands Add basic export and import commands Oct 13, 2019
@dorcastan
Copy link
Author

dorcastan commented Oct 13, 2019

@openorclose I ended up moving most of the logic into an inner class MarkImporter, which has a similar function as EditBookmarkDescriptor in EditCommand. Not sure if it should remain as an inner class or be moved somewhere else.

However, for importing Mark from Storage, I left most of the code in ImportCommand as I can't quite see how to move it to Storage. The only logic there is converting the respective Storage-related exceptions (e.g. IOException / DataConversionException) to CommandExceptions, and it seems to me that that should remain in the Command. Alternatively, for better OOP, I was thinking it could go into something like a MarkStorageHandler... But I might want to tackle that in a separate PR.

In any case, is this implementation okay for the current PR?

Copy link

@openorclose openorclose left a comment

Choose a reason for hiding this comment

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

Yup looks good

@dorcastan
Copy link
Author

@Na-Nazhou okay to merge?

v1.2 automation moved this from Review in progress to Reviewer approved Oct 14, 2019
@Na-Nazhou Na-Nazhou merged commit 09a6d69 into AY1920S1-CS2103T-T13-4:master Oct 14, 2019
v1.2 automation moved this from Reviewer approved to Done Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v1.2
  
Done
Development

Successfully merging this pull request may close these issues.

Implement import Implement basic export command
4 participants