Skip to content

Add cvt command #4

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
Merged
merged 9 commits into from
May 27, 2020
Merged

Add cvt command #4

merged 9 commits into from
May 27, 2020

Conversation

duncte123
Copy link
Member

No description provided.

MorphTuple
MorphTuple reviewed May 6, 2020
View reviewed changes
Copy link
Member

@MorphTuple MorphTuple left a comment

Choose a reason for hiding this comment

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

Add a digital data storage units (kb, mb, gb) if necessary

MorphTuple
MorphTuple previously approved these changes May 8, 2020
View reviewed changes
Copy link
Member

@MorphTuple MorphTuple left a comment

Choose a reason for hiding this comment

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

IMO It will be better if the command is used like cvt <value> <target> rather than cvt <target> <value>, but other than that the code looks good so i approve this.

Leguu
Leguu suggested changes May 8, 2020
View reviewed changes
Copy link

@Leguu Leguu left a comment

Choose a reason for hiding this comment

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

Some nitpicks, some things to consider.

@duncte123
Copy link
Member Author

IMO It will be better if the command is used like cvt <value> <target> rather than cvt <target> <value>, but other than that the code looks good so i approve this.

This was how the original command from Super Hiro was created and carried over to yuuto.
And it gets confusing if you name it value and target, just name them <unit-to-convert-to> and <input>
(and I don't want to redo the entire code to swap those two parameters)

@duncte123 duncte123 self-assigned this May 12, 2020
@duncte123 duncte123 requested a review from Leguu May 21, 2020 08:40
@duncte123 duncte123 added the review me Pull requests that need to be reviewed label May 26, 2020
MorphTuple
MorphTuple approved these changes May 27, 2020
View reviewed changes
Copy link
Member

@MorphTuple MorphTuple left a comment

Choose a reason for hiding this comment

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

LGTM

@duncte123 duncte123 removed the review me Pull requests that need to be reviewed label May 27, 2020
@duncte123 duncte123 merged commit b5d3006 into develop May 27, 2020
@duncte123 duncte123 deleted the feature/cvt branch May 27, 2020 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Leguu Leguu Leguu approved these changes

@MorphTuple MorphTuple MorphTuple approved these changes

Assignees

@duncte123 duncte123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@duncte123 @Leguu @MorphTuple