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

BUX-538: Create Paymail add description #190

Merged
merged 9 commits into from
Feb 21, 2024

Conversation

Nazarii-4chain
Copy link
Contributor

Added description

Pull Request Checklist

  • πŸ“– I created my PR using provided : CODE_STANDARDS
  • πŸ“– I have read the short Code of Conduct: CODE_OF_CONDUCT
  • 🏠 I tested my changes locally.
  • βœ… I have provided tests for my changes.
  • πŸ“ I have used conventional commits.
  • πŸ“— I have updated any related documentation.
  • πŸ’Ύ PR was issued based on the Github or Jira issue.

@Nazarii-4chain Nazarii-4chain self-assigned this Feb 2, 2024
@Nazarii-4chain Nazarii-4chain marked this pull request as ready for review February 2, 2024 11:09
@Nazarii-4chain Nazarii-4chain requested a review from a team as a code owner February 2, 2024 11:09
Copy link
Contributor

mergify bot commented Feb 2, 2024

Welcome to our open-source project @Nazarii-4chain! πŸ’˜

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Comparison is base (3a1e7e6) 59.91% compared to head (7ae3d6c) 59.91%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #190   +/-   ##
=======================================
  Coverage   59.91%   59.91%           
=======================================
  Files          16       16           
  Lines         978      978           
=======================================
  Hits          586      586           
  Misses        315      315           
  Partials       77       77           
Flag Coverage Ξ”
unittests 59.91% <ΓΈ> (ΓΈ)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Ξ”
paymail_addresses.go 100.00% <ΓΈ> (ΓΈ)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 3a1e7e6...7ae3d6c. Read the comment docs.

Copy link
Contributor

mergify bot commented Feb 5, 2024

Welcome to our open-source project @Nazarii-4chain! πŸ’˜

paymail_addresses.go Outdated Show resolved Hide resolved
Comment on lines +73 to +75
// AdminCreatePaymail Create a paymail.
//
// Paymail address (ie. [email protected])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why the comment is necessary for the AdminCreatePaymail when, at the same time, other methods (also public) are without a comment.

Besides that, the multiline comment looks to me like a separator between two groups of methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment should show an example of paymail address. Removed new line

Copy link
Contributor

Choose a reason for hiding this comment

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

But, as I see in the whole AdminService package documentation - not only the AdminCreatePaymail gets an address as string which should be a paymail.

So maybe an example how paymailAddress looks like would be in the AdminService description, like that:

// AdminService is the admin related requests
//
// Note: A Paymail address is a unique identifier for receiving payments in the
// form of a readable email-like address (e.g., [email protected]). 

https://pkg.go.dev/github.com/BuxOrg/[email protected]/transports#AdminService

image

Copy link
Contributor Author

@Nazarii-4chain Nazarii-4chain Feb 8, 2024

Choose a reason for hiding this comment

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

I've added this to show an example for users how paymail should look like.
Screenshot 2024-02-08 at 2 45 43 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the task is about just adding a comment/desc to AdminCreatePaymail - agreed, it's ok πŸ‘

But I'm seeing some inconsistency, why some methods are commented and other not. But maybe it will be resolved by next tasks in the future.

@Nazarii-4chain Nazarii-4chain merged commit 42806e5 into master Feb 21, 2024
7 checks passed
@Nazarii-4chain Nazarii-4chain deleted the BUX-538/CreatePaymailDesc branch February 21, 2024 13:08
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.

2 participants