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 Copy Command #131

Merged
merged 8 commits into from
Mar 29, 2024
Merged

Add Copy Command #131

merged 8 commits into from
Mar 29, 2024

Conversation

fivetran-tangyetong
Copy link

Currently does not support copy to clipboard functionality

Copy to clipboard functionality added alongside relevant tests

Closes #121

Currently does not support copy to clipboard functionality

Copy to clipboard functionality added alongside relevant tests
Copy link

codecov bot commented Mar 24, 2024

Codecov Report

Attention: Patch coverage is 80.76923% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 78.98%. Comparing base (15b86af) to head (552555e).

Files Patch % Lines
...java/seedu/address/logic/commands/CopyCommand.java 88.23% 6 Missing and 2 partials ⚠️
src/main/java/seedu/address/ui/MainWindow.java 0.00% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #131      +/-   ##
============================================
+ Coverage     78.89%   78.98%   +0.09%     
- Complexity      488      509      +21     
============================================
  Files            72       73       +1     
  Lines          1464     1542      +78     
  Branches        119      136      +17     
============================================
+ Hits           1155     1218      +63     
- Misses          285      298      +13     
- Partials         24       26       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@aureliony aureliony left a comment

Choose a reason for hiding this comment

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

I've added some preliminary comments, will require further analysis.

@aureliony aureliony added this to the v1.3 milestone Mar 24, 2024
Code currently does not conform to certain conventions required

Unused print statements removed and defensiveness of code maintained in new logic added
CopyCommandTest currently does not use default values in personbuilder

Default values in PersonBuilder used in test, and refactor for new line spacing
aureliony
aureliony previously approved these changes Mar 27, 2024
Copy link

@aureliony aureliony left a comment

Choose a reason for hiding this comment

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

LGTM

@aureliony aureliony dismissed their stale review March 27, 2024 14:33

copy command should only work on one field and without prepending the field name

CopyCommand returns multiple fields and field names

Update to return only field and no field name
Test coverage currently too little

Added more tests
Copy link

@aureliony aureliony left a comment

Choose a reason for hiding this comment

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

LGTM

@bryanyee33 bryanyee33 merged commit d0381f4 into master Mar 29, 2024
8 checks passed
aureliony pushed a commit that referenced this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy command
3 participants