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

merge status-message-collections #1076

Merged
merged 4 commits into from Aug 23, 2023
Merged

merge status-message-collections #1076

merged 4 commits into from Aug 23, 2023

Conversation

shariffdev
Copy link
Contributor

Addresses: #809

This PR eliminates the status-message-collection example, leaving only one status-message-esque example.

@shariffdev shariffdev marked this pull request as ready for review August 21, 2023 03:19
@shariffdev shariffdev mentioned this pull request Aug 21, 2023
Closed
uint
uint requested changes Aug 21, 2023
View reviewed changes
Copy link
Collaborator

@uint uint left a comment

Choose a reason for hiding this comment

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

Hi! Unfortunately, this is not quite what we want here. We'd like there to be one status-message contract using the LookupMap.

LookupMap is more efficient (in the NEAR blockchain context) and the appropriate storage option to choose in situations like these. Since examples need to show contract devs best practices, we really need to advertise it over HashMap here.

@shariffdev shariffdev changed the title nuke status-message-collections merge status-message-collections Aug 22, 2023
@shariffdev
Copy link
Contributor Author

Hi! Unfortunately, this is not quite what we want here. We'd like there to be one status-message contract using the LookupMap.

LookupMap is more efficient (in the NEAR blockchain context) and the appropriate storage option to choose in situations like these. Since examples need to show contract devs best practices, we really need to advertise it over HashMap here.

Understood, making chnages.

@shariffdev shariffdev requested a review from uint August 22, 2023 15:20
uint
uint approved these changes Aug 23, 2023
View reviewed changes
Copy link
Collaborator

@uint uint left a comment

Choose a reason for hiding this comment

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

LGTM!

@uint uint merged commit 83aa706 into near:master Aug 23, 2023
14 checks passed
@uint
Copy link
Collaborator

uint commented Aug 23, 2023 •
edited

@shariffdev One note: please include the phrase Closes #809 when opening a PR for a specific issue. That way the issue gets automatically closed when we merge.

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

@uint uint uint approved these changes

@frol frol Awaiting requested review from frol frol is a code owner

@agostbiro agostbiro Awaiting requested review from agostbiro agostbiro is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@shariffdev @uint