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

Web3Auth Initial documentation #673

Merged
merged 7 commits into from
Oct 30, 2023
Merged

Web3Auth Initial documentation #673

merged 7 commits into from
Oct 30, 2023

Conversation

RyRy79261
Copy link
Contributor

closes #671

@RyRy79261 RyRy79261 self-assigned this Oct 19, 2023
@RyRy79261 RyRy79261 marked this pull request as ready for review October 20, 2023 13:29
kalambet
kalambet requested changes Oct 23, 2023
View reviewed changes
Copy link
Member

@kalambet kalambet left a comment

Choose a reason for hiding this comment

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

It is not necessary to document body of the tests (no need to remove what is already there, though) but it does make sense to document bodies of the SetUp and TearDown functions more where it is necessary.
There are more tests files that are missing in this PR.

@RyRy79261
Copy link
Contributor Author

@kalambet So the test files I was pointed at were only these ones, as for any other test files, I'm not sure they're within this scope

@kalambet
Copy link
Member

@RyRy79261 what I meant is that in the ChainSafe.Tests project contains beside ProvidersSendTests.cs that you updated also:

ChainlinkLootboxTests.cs
ChainsafeRPCTests.cs
Web3Util.cs

sneakzttv
sneakzttv approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@sneakzttv sneakzttv left a comment

Choose a reason for hiding this comment

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

everything else looks great

@RyRy79261
Copy link
Contributor Author

@kalambet

that in the ChainSafe.Tests project contains beside ProvidersSendTests.cs that you updated also:

What did you mean by this?

@kalambet
Copy link
Member

I mean that this is the current set of your changes:
image

But Test project has 3 more files:

ChainlinkLootboxTests.cs
ChainsafeRPCTests.cs
Web3Util.cs

That needs docstrings.

@RyRy79261 RyRy79261 requested a review from kalambet October 26, 2023 11:18
kalambet
kalambet approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@kalambet kalambet left a comment

Choose a reason for hiding this comment

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

LGTM!

@RyRy79261 RyRy79261 merged commit b1d8edf into main Oct 30, 2023
8 checks passed
@RyRy79261 RyRy79261 deleted the ryan/web3auth-documentation-671 branch October 30, 2023 11:56
robGG1997 pushed a commit that referenced this pull request Nov 3, 2023
@RyRy79261 @robGG1997
Web3Auth Initial documentation (#673)
4a75425 
* Initial docs added

* Set up tests commented

* Added comments

* Lootbox documentation
rob1997 pushed a commit that referenced this pull request Jan 16, 2025
@RyRy79261 @rob1997
Web3Auth Initial documentation (#673)
6f1c889 
* Initial docs added

* Set up tests commented

* Added comments

* Lootbox documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robGG1997 robGG1997 robGG1997 approved these changes

@sneakzttv sneakzttv sneakzttv approved these changes

@kalambet kalambet kalambet approved these changes

@juans-chainsafe juans-chainsafe Awaiting requested review from juans-chainsafe

@kantagara kantagara Awaiting requested review from kantagara

@oleksandrchainsafe oleksandrchainsafe Awaiting requested review from oleksandrchainsafe

Assignees

@RyRy79261 RyRy79261

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@RyRy79261 @kalambet @sneakzttv @robGG1997