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

Update to use ArgumentParser #105

Merged

Conversation

ntduyet
Copy link
Contributor

@ntduyet ntduyet commented May 5, 2022

Issue number of the reported bug or feature request: #94

Describe your changes

  • Update to use ArgumentParser

Testing performed

  • All unit tests are passed.

@ntduyet ntduyet force-pushed the duyet/update-arguments-parser branch from 74d37d8 to 6230cf0 Compare May 5, 2022 16:46
@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #105 (11819d6) into main (4755a03) will increase coverage by 0.11%.
The diff coverage is 97.33%.

❗ Current head 11819d6 differs from pull request most recent head 28cb3e2. Consider uploading reports for the commit 28cb3e2 to get more accurate results

@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
+ Coverage   96.43%   96.54%   +0.11%     
==========================================
  Files          49       49              
  Lines        2719     2636      -83     
==========================================
- Hits         2622     2545      -77     
+ Misses         97       91       -6     
Impacted Files Coverage Δ
Sources/XCDiff/main.swift 0.00% <0.00%> (ø)
Sources/XCDiffCommand/CommandRunner.swift 97.36% <98.64%> (+1.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4755a03...28cb3e2. Read the comment docs.

@ntduyet ntduyet force-pushed the duyet/update-arguments-parser branch from fbcf303 to 7c2bf37 Compare May 6, 2022 08:17
Copy link
Contributor

@marciniwanicki marciniwanicki left a comment

Choose a reason for hiding this comment

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

Thanks @ntduyet , the changes look great👍 Noticed some lint errors, might be worth adjusting Xcode settings
Screenshot 2022-05-09 at 07 39 17
.

@ntduyet
Copy link
Contributor Author

ntduyet commented May 9, 2022

Thanks @marciniwanicki, hope it gets fixed now.

dmytro-anokhin
dmytro-anokhin previously approved these changes May 9, 2022
marciniwanicki
marciniwanicki previously approved these changes May 10, 2022
@marciniwanicki
Copy link
Contributor

@ntduyet since we cannot use "Squash and merge", do you mind tiding up your branch history a little bit?

  • Squashing all the commits into one
  • Updating the commit description so it matches the PR
  • Signing it

Once done, we can rebase and merge. I think the hash will still be different, but I hope we will end up with a nice git history on main.

@ntduyet
Copy link
Contributor Author

ntduyet commented May 10, 2022

@marciniwanicki sure thing

@ntduyet ntduyet force-pushed the duyet/update-arguments-parser branch 2 times, most recently from 00cad88 to 212b854 Compare May 10, 2022 09:53
Describe your changes
- Update to use ArgumentParser

Testing performed
- All unit tests are passed.

Signed-off-by: Duyet Nguyen <duyet@bloomberg.net>
@ntduyet ntduyet force-pushed the duyet/update-arguments-parser branch from 212b854 to 28cb3e2 Compare May 10, 2022 09:54
@marciniwanicki marciniwanicki merged commit bcb6c64 into bloomberg:main May 10, 2022
@kwridan kwridan mentioned this pull request Jul 4, 2022
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.

None yet

3 participants