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

chore(doc): Add comments and derive attributes #60

Merged
merged 33 commits into from
Apr 28, 2024
Merged

chore(doc): Add comments and derive attributes #60

merged 33 commits into from
Apr 28, 2024

Conversation

malik672
Copy link
Owner

this is to solve the documentation issue and close issue #50 and

@malik672 malik672 requested a review from shuhuiluo March 22, 2024 16:48
Copy link
Collaborator

@shuhuiluo shuhuiluo left a comment

Choose a reason for hiding this comment

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

Do not base the doc changes on the big-decimal changes as it changes the behaviour of this SDK. The amount of comments is excessive especially in the addresses mod since most of the variables are self-explanatory.

Comment on lines +30 to +33
/// Contains commonly used items from the Uniswap SDK Core.
///
/// This module re-exports items that are commonly used together,
/// making it easier to import them in other parts of your application.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These comments should be placed on the top of each module.

src/entities/mod.rs Outdated Show resolved Hide resolved
Comment on lines 1 to 10
/// This module contains functionality related to currency amounts in the context of fractions.
pub mod currency_amount;
/// This module contains functionality related to fractions, which are used for various calculations
/// in the SDK.
pub mod fraction;
/// This module contains functionality related to percentages, which are used for expressing
/// fractions in a more human-readable format.
pub mod percent;
/// This module contains functionality related to prices, which are used for calculating exchange
/// rates and other financial metrics.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These comments are so AI that they are too general and don't provide much insight.

Copy link
Owner Author

@malik672 malik672 Mar 23, 2024

Choose a reason for hiding this comment

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

true, will still have to change them just wanted something to fill the comments

@malik672
Copy link
Owner Author

Do not base the doc changes on the big-decimal changes as it changes the behaviour of this SDK. The amount of comments is excessive especially in the addresses mod since most of the variables are self-explanatory.

tbh I didn't do it myself, used phind to generate most of the comments

@malik672 malik672 requested a review from shuhuiluo March 24, 2024 18:25
Copy link
Collaborator

@shuhuiluo shuhuiluo left a comment

Choose a reason for hiding this comment

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

I don't mean to waste your time. But could you start a new PR and only add comments where the code is not self-explanatory?

@malik672
Copy link
Owner Author

I don't mean to waste your time. But could you start a new PR and only add comments where the code is not self-explanatory

sure, point me to the parts

@shuhuiluo
Copy link
Collaborator

I don't mean to waste your time. But could you start a new PR and only add comments where the code is not self-explanatory

sure, point me to the parts

Use your judgement. Break down the changes.

@malik672 malik672 requested a review from shuhuiluo April 13, 2024 13:07
@malik672 malik672 added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 13, 2024
This was linked to issues Apr 13, 2024
Copy link
Collaborator

@shuhuiluo shuhuiluo left a comment

Choose a reason for hiding this comment

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

As I said, start a new PR and only add comments where the code isn't self-explanatory. Do not fill the code with useless AI comments to reduce readability. Move the hash map change to a different PR.

@malik672
Copy link
Owner Author

malik672 commented Apr 14, 2024

As I said, start a new PR and only add comments where the code isn't self-explanatory. Do not fill the code with useless AI comments to reduce readability. Move the hash map change to a different PR.

yeah true, but there would have to be documentation though even for the self explanatory ones(from alloy) and can it not just be possible to just do both in one pr and maybe just change the title

@shuhuiluo
Copy link
Collaborator

As I said, start a new PR and only add comments where the code isn't self-explanatory. Do not fill the code with useless AI comments to reduce readability. Move the hash map change to a different PR.

yeah true, but there would have to be documentation though even for the self explanatory ones(from alloy) and can it not just be possible to just do both in one pr and maybe just change the title

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/getting-started/best-practices-for-pull-requests

@malik672 malik672 requested a review from shuhuiluo April 17, 2024 14:13
src/addresses.rs Outdated Show resolved Hide resolved
src/addresses.rs Outdated Show resolved Hide resolved
src/chains.rs Outdated Show resolved Hide resolved
src/chains.rs Outdated Show resolved Hide resolved
src/constants.rs Outdated Show resolved Hide resolved
src/entities/token.rs Outdated Show resolved Hide resolved
src/entities/token.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/utils/mod.rs Outdated Show resolved Hide resolved
@shuhuiluo shuhuiluo changed the title Docs chore(doc): Add comments and derive attributes Apr 21, 2024
malik672 and others added 17 commits April 22, 2024 22:08
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
src/error.rs Outdated Show resolved Hide resolved
Co-authored-by: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com>
@malik672 malik672 requested a review from shuhuiluo April 28, 2024 07:44
@shuhuiluo shuhuiluo merged commit 6c050a0 into master Apr 28, 2024
1 of 2 checks passed
@shuhuiluo shuhuiluo deleted the docs branch April 28, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a faster HashMap crate Enhance documentation, add doctest
2 participants