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

release: 0.2 #104

Merged
merged 14 commits into from
Apr 6, 2022
Merged

release: 0.2 #104

merged 14 commits into from
Apr 6, 2022

Conversation

ChaoticTempest
Copy link
Member

No description provided.

@ChaoticTempest ChaoticTempest requested review from itegulov and removed request for itegulov April 5, 2022 02:05
workspaces/Cargo.toml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ChaoticTempest ChaoticTempest marked this pull request as ready for review April 5, 2022 23:13
workspaces/Cargo.toml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

ignoring wording nits on README, looks good to be released

README.md Outdated
## Testing
A simple test to get us going and familiar with the features:
### Note about compiling with contract
`workspaces-rs` does not currently support compiling to WASM. So, if we are compiling this library alongside a `wasm32` target such as compiling alongside a NEAR contract, it is best if we put this dependency in `[dev-dependencies]` section to avoid any conflicts.

Choose a reason for hiding this comment

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

Discussed in chat. But I initially read this as a reference to the contract code being complied rather than workspaces itself

Copy link
Member Author

Choose a reason for hiding this comment

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

@doriancrutcher @AustinBaggio should look better now

README.md Outdated Show resolved Hide resolved
Copy link

@doriancrutcher doriancrutcher left a comment

Choose a reason for hiding this comment

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

Only added a note for clarification of one section and mentioned some typos but overall I think it looks good!

@ChaoticTempest ChaoticTempest merged commit f2dcf76 into main Apr 6, 2022
@ChaoticTempest ChaoticTempest deleted the release/0.2 branch April 6, 2022 01:01
```toml
# Cargo.toml
[dependencies]
workspaces = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we showing an example of a broken pattern in the readme?

Copy link
Member Author

@ChaoticTempest ChaoticTempest Apr 6, 2022

Choose a reason for hiding this comment

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

Just trying to show the dichotomy of how it'd look like if it didn't work vs the working example. Any suggestions to make it better or to reword it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This just seems like something relating to the SDK. I feel like if we just share that it does not compile to wasm with some information about that would be sufficient, rather than showing an example of workspaces being included in the SDK dependencies (which it should never be).

Opinionated so nothing has to change, just sharing thoughts that it might be confusing to someone reading

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, yeah that's a good point. I still mentioned the SDK since that seems to be the most common case people would hit from what I've seen. Added the fix here: #115

README.md Show resolved Hide resolved
@frol frol mentioned this pull request Oct 4, 2023
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.

5 participants