Impr docs #1262

PolyProgrammist · 2024-11-20T13:55:30Z

No description provided.

codecov · 2024-11-20T14:05:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.16%. Comparing base (ae0d1c4) to head (b1b185f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1262      +/-   ##
==========================================
- Coverage   80.26%   80.16%   -0.11%     
==========================================
  Files         104      104              
  Lines       14855    14855              
==========================================
- Hits        11924    11909      -15     
- Misses       2931     2946      +15

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

dj8yfo · 2024-11-29T09:56:31Z

near-sdk/src/environment/env.rs

@@ -500,6 +500,8 @@ pub fn alt_bn128_pairing_check(value: &[u8]) -> bool {
 // ################
 /// Creates a promise that will execute a method on account with given arguments and attaches
 /// the given amount and gas.


@PolyProgrammist suggest adding more links, mentioned in #1265 and marked up in review of #1259,
in addition to ones, added in scope of current pr, after fixing conflicts, if you plan to finish current pr

Which links do you mean? @dj8yfo

Added half of the comments. Left the other for the future

denbite

Great job on this, I've dropped a few comments with some thoughts on how it could be even better
it's mainly focused on adding more examples

near-sdk-macros/src/lib.rs

near-sdk/src/lib.rs

PolyProgrammist · 2024-12-20T17:18:06Z

@dj8yfo @denbite thanks for the review. Please check again

dj8yfo · 2025-01-06T14:47:12Z

near-sdk/src/environment/env.rs

@@ -159,7 +159,7 @@ pub fn register_len(register_id: u64) -> Option<u64> {
 /// use near_sdk::AccountId;
 /// use std::str::FromStr;
 ///
-/// assert_eq!(current_account_id(), AccountId::from_str("alice.near").unwrap());
+/// assert_eq!(current_account_id(), "alice.near".parse().unwrap());


adding turbofishes helps in this and similar occasions helps

diff --git a/near-sdk/src/environment/env.rs b/near-sdk/src/environment/env.rs index f04121cd..28bd0e75 100644 --- a/near-sdk/src/environment/env.rs +++ b/near-sdk/src/environment/env.rs @@ -152,21 +152,21 @@ pub fn register_len(register_id: u64) -> Option<u64> { // # Context API # // ############### /// The id of the account that owns the current contract. /// /// # Examples /// ``` /// use near_sdk::env::current_account_id; /// use near_sdk::AccountId; /// use std::str::FromStr; /// -/// assert_eq!(current_account_id(), "alice.near".parse().unwrap()); +/// assert_eq!(current_account_id(), "alice.near".parse::<AccountId>().unwrap()); /// ``` pub fn current_account_id() -> AccountId { assert_valid_account_id(method_into_register!(current_account_id)) } /// The id of the account that either signed the original transaction or issued the initial /// cross-contract call. /// /// # Examples /// ```

akorchyn

Overall good job. However, I added a couple nits and one major point to fix (use the near macro in root macro)

akorchyn · 2025-01-15T13:38:39Z

near-sdk-macros/src/lib.rs

+/// # Parameter and result serialization
+/// If the macro is used with Impl section, for parameter serialization, this macro will generate a struct with all of the parameters as
+/// fields and derive deserialization for it. By default this will be JSON deserialized with `serde`
+/// but can be overwritten by using `#[serializer(borsh)]`:
 /// ```ignore


add rust tag

akorchyn · 2025-01-15T13:38:46Z

near-sdk-macros/src/lib.rs

 ///
-/// # Example
+/// ## Example
 ///
 /// ```ignore


this is probably not possible as is,
doing

/// ```rust /// use near_sdk_macros::near; /// /// #[near(serializers=[borsh, json])] /// struct MyStruct { /// pub name: String, /// } /// ```

results in

---- near-sdk-macros/src/lib.rs - near (line 55) stdout ---- error[E0433]: failed to resolve: could not find `near_sdk` in the list of imported crates --> near-sdk-macros/src/lib.rs:59:1 | 7 | #[near(serializers=[borsh, json])] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ could not find `near_sdk` in the list of imported crates | = note: this error originates in the attribute macro `near` (in Nightly builds, run with -Z macro-backtrace for more info) error: cannot find attribute `borsh` in this scope --> near-sdk-macros/src/lib.rs:59:1 | 7 | #[near(serializers=[borsh, json])] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the attribute macro `near` (in Nightly builds, run with -Z macro-backtrace for more info) error: cannot find attribute `serde` in this scope --> near-sdk-macros/src/lib.rs:59:1 | 7 | #[near(serializers=[borsh, json])] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `serde` is in scope, but it is a crate, not an attribute = note: this error originates in the attribute macro `near` (in Nightly builds, run with -Z macro-backtrace for more info) error: aborting due to 3 previous errors

this is why #1288 was suggested

alternatively to #1288, a circular [dev-dependency] appears to allow this too:

diff --git a/near-sdk-macros/Cargo.toml b/near-sdk-macros/Cargo.toml index 1f0388d4..31974b8d 100644 --- a/near-sdk-macros/Cargo.toml +++ b/near-sdk-macros/Cargo.toml @@ -21,19 +21,20 @@ strum = { version = "0.26", default-features = false } strum_macros = "0.26" quote = { version = "1.0", default-features = false } Inflector = { version = "0.11.4", default-features = false, features = [] } darling = { version = "0.20.3", default-features = false } serde = { version = "1", default-features = false, features = ["serde_derive"] } serde_json = "1" [dev-dependencies] insta = { version = "1.31.0", features = ["yaml"] } prettyplease = { version = "0.2.15" } +near-sdk = { path = "../near-sdk" } [features] abi = [] __abi-embed = ["abi"] __abi-generate = ["abi"] [package.metadata.docs.rs] features = ["__abi-generate"]

not sure if this entails additional problems, but this looks a one-liner compared to moving larger blocks of code.
And near-sdk kind of re-exports all of its dependencies it uses for derive macros (does it?)
Please leave a comment on #1288, if you decide to try a circular [dev-dependency] instead

To be honest, I agree that it's better to document on near sdk level. I doubt that a lot of people will go in the macro crate at all...

the doc will be re-exported from near-sdk-macros anyway, as it is now

Another example/explanation: doc at https://docs.rs/borsh/1.5.4/borsh/derive.BorshSerialize.html gets concatenated of 2 pieces: one at doc-comment at site of re-export, one at original doc-comment site:

the only subtlety i see , is that if one does

[dev-dependencies] near-sdk = { path = "../near-sdk" , features = "__abi-generate" }

then near-sdk/__abi-generate => near-sdk-macros/__abi-generate feature becomes implicitly enabled
when anyone runs cargo test for near-sdk-macros crate (which wouldn't be otherwise enabled) , but i think it's kind of ok here.
so it looks like trying a one-liner is simpler, than moving a few blocks of code.
Anyway, @akorchyn should make a final call for the pr here, as he owns the review here

(i'm pro #1288 approach , doc at reexport in near-sdk should describe how to use the macros,
doc in near-sdk-macros of various non-public items might explain how the macros work internally, if someone wants to understand/extend them, the latter is only possible when documenting private items is enabled for proc-macro crates)

I like the #1288 approach more, to be honest.

akorchyn · 2025-01-15T13:39:18Z

near-sdk-macros/src/lib.rs

+/// function execution based on what type is returned by the function. By default, this will be
+/// done through `serde` serialized as JSON, but this can be overwritten using
+/// `#[result_serializer(borsh)]`:
+/// ```ignore


akorchyn · 2025-01-15T13:40:01Z

near-sdk-macros/src/lib.rs

-/// # Example
+/// If you want the struct to be a contract state, you can pass in the contract_state argument.
+///
+/// ## Example
 /// ```ignore


akorchyn · 2025-01-15T13:49:59Z

near-sdk/src/lib.rs

+//! use near_sdk::borsh::{self, BorshDeserialize, BorshSerialize};
+//! use near_sdk::{env, near_bindgen};
+//!
+//! #[near_bindgen]


Why do we use near bindgen here?
It looks a bit out of scope. As we stated before it is deprecated. (even though, deprecated is the wrong word and more like low-level). We should definitely use here a new near syntax given the fact that it is the primary point for the doc.

akorchyn · 2025-01-15T13:51:41Z

near-sdk/src/store/mod.rs

@@ -70,6 +70,8 @@
 //! - [`LazyOption<T>`](LazyOption): Lazily loaded, optional type that can be used in
 //!   place of a type [`Option<T>`](Option). Will only be loaded when interacted with and will
 //!   persist on [`Drop`].
+//!
+//! More information about collections can be found in [NEAR documentation](https://docs.near.org/build/smart-contracts/anatomy/collections)


There was somewhere performance estimation that would be nice to link

akorchyn · 2025-01-15T13:52:45Z

near-sdk/src/types/mod.rs

@@ -8,7 +8,9 @@ mod primitives;
 pub use self::primitives::*;

 pub use near_account_id::{AccountId, AccountIdRef};
+/// Same as `NearGas` from crate [`near_gas`](near_gas)


Alias to near_gas::NearGas. I believe, it should derive the docs from original type, so it is not really necessary

PolyProgrammist added 9 commits November 19, 2024 22:30

tmp

7d18cd0

tmp

3803218

nearschema

d1015f0

link for store crate, neargas and neartoken types

245c1c9

promises in env

34d774f

fmt and env intro

5f0aeb1

promise_index and promise structs

ed0bdef

storage and functionerror

589d61c

small

1911c50

dj8yfo reviewed Nov 29, 2024

View reviewed changes

denbite reviewed Dec 11, 2024

View reviewed changes

PolyProgrammist added 13 commits December 20, 2024 20:32

denbite review

b6951e7

Merge branch 'master' into impr-docs

3b1ce54

comments from PR 1259

feaa181

fmt

8d178bd

clippy

aef8c14

fmt

49bc57b

docs test fix

f29575c

fix doc

48ccae6

fix docs

4d92375

fix docs

63b8b0a

fix

d46cd16

fix

9bbd19a

Merge branch 'master' into impr-docs

08dd4ff

PolyProgrammist marked this pull request as ready for review December 20, 2024 17:15

PolyProgrammist requested review from frol, ruseinov and akorchyn as code owners December 20, 2024 17:15

PolyProgrammist added 9 commits December 27, 2024 04:00

fix ci test

99c7e13

Merge branch 'master' into impr-docs

e1c6c57

fmt

22e4098

ignore to no_run

c63918d

remove unnecessary near-primitives and near-vm-runner for docs

c2732a2

fix no-run tests

f0c05da

fmt

8e16e83

fix tests

916e81d

fmt

3d5cb4e

dj8yfo mentioned this pull request Dec 28, 2024

doc: add cargo test --doc --features __macro-docs near-sdk/src/near.rs to test job #1287

Closed

PolyProgrammist added 2 commits December 30, 2024 01:02

fix tests

bc34ace

checkj

b1b185f

dj8yfo reviewed Jan 6, 2025

View reviewed changes

akorchyn requested changes Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Impr docs #1262

Impr docs #1262

PolyProgrammist commented Nov 20, 2024

codecov bot commented Nov 20, 2024 •

edited

Loading

dj8yfo Nov 29, 2024

PolyProgrammist Dec 20, 2024

PolyProgrammist Dec 20, 2024

denbite left a comment •

edited

Loading

PolyProgrammist commented Dec 20, 2024

dj8yfo Jan 6, 2025 •

edited

Loading

akorchyn left a comment

akorchyn Jan 15, 2025

akorchyn Jan 15, 2025

dj8yfo Jan 15, 2025

dj8yfo Jan 15, 2025 •

edited

Loading

akorchyn Jan 15, 2025

dj8yfo Jan 15, 2025 •

edited

Loading

akorchyn Jan 16, 2025

akorchyn Jan 15, 2025

akorchyn Jan 15, 2025

akorchyn Jan 15, 2025

akorchyn Jan 15, 2025

akorchyn Jan 15, 2025

Impr docs #1262

Are you sure you want to change the base?

Impr docs #1262

Conversation

PolyProgrammist commented Nov 20, 2024

codecov bot commented Nov 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denbite left a comment • edited Loading

Choose a reason for hiding this comment

PolyProgrammist commented Dec 20, 2024

dj8yfo Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

akorchyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dj8yfo Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dj8yfo Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 20, 2024 •

edited

Loading

denbite left a comment •

edited

Loading

dj8yfo Jan 6, 2025 •

edited

Loading

dj8yfo Jan 15, 2025 •

edited

Loading

dj8yfo Jan 15, 2025 •

edited

Loading