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

feat: replace and replaceGet map methods & refactor of codegen and testing #941

Merged
merged 25 commits into from
Oct 31, 2024
Merged

feat: replace and replaceGet map methods & refactor of codegen and testing #941

merged 25 commits into from
Oct 31, 2024

Conversation

Gusarich
Copy link
Member

@Gusarich Gusarich commented Oct 11, 2024 •
edited
Loading

Issue

Closes #538.

Checklist

  • I have updated CHANGELOG.md
  • I have documented my contribution in docs/ and made the build locally
  • I have added tests to demonstrate the contribution is correctly implemented: this usually includes both positive and negative tests, showing the happy path(s) and featuring intentionally broken cases
  • I have run all the tests locally and no test failure was reported
  • I have run the linter, formatter and spellchecker
  • I did not do unrelated and/or undiscussed refactorings

@Gusarich Gusarich added this to the v1.6.0 milestone Oct 11, 2024
@Gusarich Gusarich marked this pull request as ready for review October 15, 2024 22:48
@Gusarich Gusarich requested a review from a team as a code owner October 15, 2024 22:48
@Gusarich Gusarich changed the title feat: implement map.replace feat: implement replace and replaceGet map methods Oct 15, 2024
@anton-trunov anton-trunov self-assigned this Oct 16, 2024
anton-trunov
anton-trunov requested changes Oct 16, 2024
View reviewed changes
Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

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

Cool stuff! Let's do the suggested refactoring. And also add some simple docs, unless @novusnota is willing to volunteer to help with that

src/abi/map.ts Outdated Show resolved Hide resolved
src/abi/map.ts Show resolved Hide resolved
@novusnota
Copy link
Member

novusnota commented Oct 16, 2024 •
edited
Loading

unless @novusnota is willing to volunteer to help with that

Yup, will do tomorrow soon, aside from other doc issues :)

jeshecdom
jeshecdom reviewed Oct 17, 2024
View reviewed changes
src/test/e2e-emulated/contracts/maps.tact Outdated Show resolved Hide resolved
src/test/e2e-emulated/map.spec.ts Outdated Show resolved Hide resolved
src/test/e2e-emulated/map.spec.ts Outdated Show resolved Hide resolved
src/test/e2e-emulated/map.spec.ts Outdated Show resolved Hide resolved
src/test/e2e-emulated/map.spec.ts Outdated Show resolved Hide resolved
src/test/e2e-emulated/map.spec.ts Outdated Show resolved Hide resolved
@novusnota novusnota mentioned this pull request Oct 22, 2024
Closed
3 tasks
@Gusarich Gusarich changed the title feat: implement replace and replaceGet map methods feat: implement replace and replaceGet map methods and refactor map.ts Oct 25, 2024
@Gusarich Gusarich mentioned this pull request Oct 25, 2024
Merged
5 tasks
@Gusarich
Copy link
Member Author

@anton-trunov @jeshecdom have a look please!

map tests are now extremely large and slow so I want to refactor them completely in a separate PR after this one gets merged.
Well, I can do that in this one too if you want it that way

jeshecdom
jeshecdom reviewed Oct 26, 2024
View reviewed changes
src/test/e2e-emulated/map.spec.ts Outdated Show resolved Hide resolved
src/test/e2e-emulated/map.spec.ts Outdated Show resolved Hide resolved
@Gusarich Gusarich changed the title feat: implement replace and replaceGet map methods and refactor map.ts feat: replace and replaceGet map methods & refactor of codegen and testing Oct 27, 2024
@Gusarich Gusarich mentioned this pull request Oct 29, 2024
Merged
3 tasks
@Gusarich
Copy link
Member Author

Gusarich commented Oct 29, 2024 •
edited
Loading

@anton-trunov @jeshecdom have a look at the final tests please!

not added a few special cases from the original implementation yet, but all the operations are tested now on multiple sets of key-value pairs. just want to make sure to include all the important scenarios.

jeshecdom
jeshecdom reviewed Oct 30, 2024
View reviewed changes
Copy link
Contributor

@jeshecdom jeshecdom left a comment

Choose a reason for hiding this comment

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

The tests are awesome! I just added a small comment below.

src/test/e2e-emulated/map.spec.ts Show resolved Hide resolved
Gusarich and others added 20 commits October 31, 2024 01:34
@Gusarich
chore: update changelog
4cb46c3
@Gusarich
chore: update cspell
6a47645
@Gusarich @anton-trunov
chore: remove should not happen comment
8428344 
Co-authored-by: Anton Trunov <anton.a.trunov@gmail.com>
@novusnota @Gusarich
feat(docs): moved from PR tact-lang#997
cbb8d08
@Gusarich
feat: refactor maps operations
9b56f2e
@Gusarich
fix: replaceGet codegen fix for structs
5e1fd0c
@Gusarich
feat: cover all maps with tests
d7ca167
@Gusarich
feat: rework maps testing contract completely to cover all different …
95e32c5 
…key-value combinations and all methods
@Gusarich
feat: wip on typescript testing part (.set and .get)
fa0598d
@Gusarich
fix: keys in DelAllMaps operation
5181ca7
@Gusarich
feat: test .del operation
33f5ad6
@Gusarich
feat: add multiple sets of key-value pairs for testing
d8026cb
@Gusarich
feat: add more .set test cases
52a330f
@Gusarich
feat: add more .get test cases
22f56cc
@Gusarich
feat: add more .del test cases
e1113ba
@Gusarich
feat: add .exists test cases
def2c16
@Gusarich
feat: add .isEmpty and .asCell test cases
96d2f72
@Gusarich
feat: .replace and .replaceGet test cases (wip)
03cfb3b
@Gusarich
feat: add replaceGet getter test case
a9ee943
@Gusarich
feat: null reference exception test case
523b401
jeshecdom
jeshecdom reviewed Oct 30, 2024
View reviewed changes
Copy link
Contributor

@jeshecdom jeshecdom left a comment

Choose a reason for hiding this comment

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

Tests LGTM.

docs/src/content/docs/book/maps.mdx Show resolved Hide resolved
docs/src/content/docs/book/maps.mdx Show resolved Hide resolved
@anton-trunov anton-trunov merged commit b571ec1 into tact-lang:main Oct 31, 2024
16 checks passed
@Gusarich Gusarich mentioned this pull request Nov 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeshecdom jeshecdom jeshecdom left review comments

@anton-trunov anton-trunov anton-trunov approved these changes

@novusnota novusnota Awaiting requested review from novusnota

Assignees

@anton-trunov anton-trunov

Labels
None yet
Projects
None yet
Milestone
v1.6.0
Development

Successfully merging this pull request may close these issues.

Replace map operations
4 participants
@Gusarich @novusnota @jeshecdom @anton-trunov