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.

Sign up for GitHub

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

Jump to bottom

Added ConvertCommand with tests #168

Merged
merged 4 commits into from Oct 22, 2019
Merged

Added ConvertCommand with tests #168

merged 4 commits into from Oct 22, 2019

Conversation

krusagiz
Copy link

@krusagiz krusagiz commented Oct 21, 2019 •
edited by limerencee

Added ConvertCommand

Tests are included.

Resolves #34 Resolves #66 Resolves #130

@krusagiz krusagiz added this to the v1.3 milestone Oct 21, 2019
lightz96
lightz96 requested changes Oct 21, 2019
View reviewed changes
Copy link

@lightz96 lightz96 left a comment •
edited

Choose a reason for hiding this comment

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

Unable to run the application. You should allow the creation of the JSON file if JSON file is missing.

@krusagiz
Added ConvertCommand with tests
13347ff 
- Allow for corrupted or empty currency.json
@krusagiz
Copy link
Author

Unable to run the application. You should allow the creation of the JSON file if JSON file is missing.

This problem has been resolved. It now runs even without the JSON file, does not write a new currency file, however.

lightz96
lightz96 requested changes Oct 22, 2019
View reviewed changes
Copy link

@lightz96 lightz96 left a comment

Choose a reason for hiding this comment

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

LGTM. This PR can merge once minor coding standards issues are fixed.

src/main/java/thrift/logic/commands/ConvertCommand.java Show resolved Hide resolved
src/main/java/thrift/logic/commands/ConvertCommand.java Show resolved Hide resolved
src/main/java/thrift/logic/parser/ParserUtil.java Show resolved Hide resolved
src/main/java/thrift/model/util/CurrencyUtil.java Outdated Show resolved Hide resolved
src/main/java/thrift/model/util/CurrencyUtil.java Show resolved Hide resolved
limerencee
limerencee requested changes Oct 22, 2019
View reviewed changes
Copy link

@limerencee limerencee left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome feature 😄

src/main/java/thrift/logic/commands/ConvertCommand.java Outdated Show resolved Hide resolved
src/main/java/thrift/logic/parser/CliSyntax.java Outdated Show resolved Hide resolved
@krusagiz
Added DECIMAL_FORMATTER to format values
7f3501d 
- Rearranged CliSyntax variables in alphabetical order
@limerencee limerencee merged commit 8987a55 into AY1920S1-CS2103T-W12-2:master Oct 22, 2019
@krusagiz krusagiz deleted the convert-command branch October 22, 2019 04:42
@krusagiz krusagiz restored the convert-command branch October 22, 2019 12:01
@nus-se-bot nus-se-bot mentioned this pull request Oct 22, 2019
Closed
23 tasks
@krusagiz krusagiz deleted the convert-command branch October 28, 2019 12:27
@nus-se-bot nus-se-bot mentioned this pull request Oct 29, 2019
Closed
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@limerencee limerencee limerencee approved these changes

@lightz96 lightz96 lightz96 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.3
3 participants
@krusagiz @limerencee @lightz96