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

refactor(schema)!: rename ObjectType -> StateObjectType #21691

Merged
merged 6 commits into from
Sep 16, 2024

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Sep 12, 2024

Description

This does a bunch of renaming of schema.ObjectType to schema.StateObjectType - the reasoning being that in the context of VMs and programming in general, Object has a much more generic meaning than this specific meaning we have here of some data in state represented by key-value pair fields.

I don't love needing to do these sorts of renamings, but I think this is hopefully less confusing... Also open to sticking with the existing name if it's not a problem or changing this to an alternate name such as CollectionType.

Either way if we are going to do a renaming, I'd rather get the breakage done with ASAP.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features
    • Enhanced handling of state-related objects with updated types and methods.
  • Bug Fixes
    • Improved type safety and clarity in object handling across various modules.
  • Documentation
    • Updated comments and documentation to reflect changes from ObjectType to StateObjectType and ObjectUpdate to StateObjectUpdate.
  • Tests
    • Refactored tests to align with the new state object paradigm, ensuring accurate validation and functionality.

Copy link
Contributor

coderabbitai bot commented Sep 12, 2024

Walkthrough

Walkthrough

The changes involve a comprehensive refactoring of the codebase to transition from using general object types (ObjectType, ObjectUpdate) to more specific state-related types (StateObjectType, StateObjectUpdate). This includes updates across multiple files, affecting function signatures, variable types, and method returns to enhance type specificity and clarity in handling state management within the application.

Changes

Files Change Summary
indexer/postgres/create_table_test.go Changed parameter types from ObjectType to StateObjectType in functions exampleCreateTable and exampleCreateTableOpt.
indexer/postgres/internal/testdata/example_schema.go Updated global variables from ObjectType to StateObjectType.
indexer/postgres/module.go Modified initializeSchema to use StateObjectTypes instead of ObjectTypes.
indexer/postgres/object.go Changed objectIndexer struct's typ field from ObjectType to StateObjectType.
indexer/postgres/select.go Updated return types in get and readRow methods from ObjectUpdate to StateObjectUpdate.
indexer/postgres/view.go Changed return types in methods to use StateObjectType and StateObjectUpdate.
schema/appdata/data.go Updated Updates field in ObjectUpdateData struct from ObjectUpdate to StateObjectUpdate.
schema/decoder.go Renamed KVDecoder type to handle StateObjectUpdate instead of ObjectUpdate.
schema/decoding/decoding_test.go Transitioned tests to use StateObjectUpdate instead of ObjectUpdate.
schema/decoding/resolver_test.go Updated methods in modA and modB to use StateObjectType.
schema/diff/diff.go Renamed fields in ModuleSchemaDiff to focus on state object types.
schema/diff/diff_test.go Updated tests to reflect changes from ObjectType to StateObjectType.
schema/diff/state_object_diff.go Renamed ObjectTypeDiff to StateObjectTypeDiff and updated associated methods.
schema/diff/state_object_diff_test.go Updated tests to use StateObjectTypeDiff.
schema/fields.go Updated documentation to refer to StateObjectUpdate.
schema/module_schema.go Changed validation and lookup methods to use StateObjectType.
schema/state_object.go Renamed ObjectType to StateObjectType and updated related methods.
schema/state_object_test.go Transitioned tests to use StateObjectType and StateObjectUpdate.
schema/state_object_update.go Renamed ObjectUpdate to StateObjectUpdate.
schema/testing/appdatasim/app_data.go Updated formatUpdateKey function to use StateObjectUpdate.
schema/testing/example_schema.go Updated schema entries and mkTestObjectType to use StateObjectType.
schema/testing/fmt.go Changed ObjectKeyString to accept StateObjectType.
schema/testing/module_schema.go Modified to use StateObjectTypeGen.
schema/testing/object.go Renamed object generation functions to use state-specific types.
schema/testing/object_test.go Updated tests to reflect state-specific object generation.
schema/testing/statesim/app.go Changed updates slice type from ObjectUpdate to StateObjectUpdate.
schema/testing/statesim/module.go Updated module methods to handle StateObjectUpdate.
schema/testing/statesim/object_coll.go Transitioned ObjectCollection to use state-specific types.
schema/type.go Renamed methods and interfaces to reflect state object types.
schema/view/object.go Updated ObjectCollection methods to use StateObjectUpdate.

Possibly related PRs


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@aaronc aaronc marked this pull request as ready for review September 12, 2024 18:42
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Outside diff range and nitpick comments (1)
schema/state_object_test.go (1)

Line range hint 136-150: Approved with a suggestion.

Using StateObjectType in this test case is consistent with the new type definition. Testing the behavior when an object type has a duplicate key field name is important for maintaining data integrity.

However, the test case name "duplicate field 22" seems arbitrary. Consider renaming it to something more descriptive like "duplicate key field name" to enhance the readability and maintainability of the test suite.

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bd52dcf and ba32357.

Files selected for processing (32)
  • indexer/postgres/create_table_test.go (1 hunks)
  • indexer/postgres/internal/testdata/example_schema.go (3 hunks)
  • indexer/postgres/module.go (1 hunks)
  • indexer/postgres/object.go (1 hunks)
  • indexer/postgres/select.go (5 hunks)
  • indexer/postgres/view.go (1 hunks)
  • schema/appdata/data.go (1 hunks)
  • schema/decoder.go (1 hunks)
  • schema/decoding/decoding_test.go (11 hunks)
  • schema/decoding/resolver_test.go (3 hunks)
  • schema/diff/diff.go (4 hunks)
  • schema/diff/diff_test.go (12 hunks)
  • schema/diff/state_object_diff.go (3 hunks)
  • schema/diff/state_object_diff_test.go (4 hunks)
  • schema/fields.go (1 hunks)
  • schema/module_schema.go (5 hunks)
  • schema/module_schema_test.go (14 hunks)
  • schema/state_object.go (3 hunks)
  • schema/state_object_test.go (18 hunks)
  • schema/state_object_update.go (2 hunks)
  • schema/testing/appdatasim/app_data.go (1 hunks)
  • schema/testing/example_schema.go (8 hunks)
  • schema/testing/fmt.go (1 hunks)
  • schema/testing/fmt_test.go (2 hunks)
  • schema/testing/module_schema.go (1 hunks)
  • schema/testing/object.go (5 hunks)
  • schema/testing/object_test.go (1 hunks)
  • schema/testing/statesim/app.go (1 hunks)
  • schema/testing/statesim/module.go (4 hunks)
  • schema/testing/statesim/object_coll.go (3 hunks)
  • schema/type.go (4 hunks)
  • schema/view/object.go (1 hunks)
Files skipped from review due to trivial changes (2)
  • schema/fields.go
  • schema/state_object_update.go
Additional context used
Path-based instructions (30)
schema/testing/object_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

schema/testing/module_schema.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

indexer/postgres/object.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/testing/fmt.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/view/object.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/testing/fmt_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

indexer/postgres/module.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/decoder.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

indexer/postgres/internal/testdata/example_schema.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/type.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

indexer/postgres/create_table_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

indexer/postgres/view.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/testing/statesim/module.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/decoding/resolver_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

schema/testing/object.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/testing/statesim/app.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/testing/example_schema.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/state_object.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/diff/state_object_diff.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/testing/statesim/object_coll.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/diff/diff.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/appdata/data.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/module_schema.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/state_object_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

schema/testing/appdatasim/app_data.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

indexer/postgres/select.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

schema/module_schema_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

schema/diff/state_object_diff_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

schema/diff/diff_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

schema/decoding/decoding_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

Additional comments not posted (147)
schema/testing/object_test.go (2)

12-12: LGTM!

The change to use StateObjectTypeGen aligns with the overall refactoring and the test still validates the generated object type correctly.


19-21: LGTM!

The changes to use StateObjectTypeGen and StateObjectInsertGen align with the overall refactoring. The test still generates and validates the object type and update correctly, maintaining sufficient coverage.

schema/testing/module_schema.go (1)

19-19: LGTM! Verify ModuleSchemaGen's behavior with unit tests.

The change from ObjectTypeGen to StateObjectTypeGen aligns with the broader refactoring to StateObjectType. It's a valid modification within the ModuleSchemaGen function.

However, as this function generates test schemas, it's crucial to verify that its behavior remains as expected with this change. Please ensure there are sufficient unit tests covering the various scenarios handled by ModuleSchemaGen to catch any potential issues introduced by using StateObjectTypeGen.

indexer/postgres/object.go (2)

12-12: LGTM!

The change from schema.ObjectType to schema.StateObjectType for the typ field in the objectIndexer struct is consistent with the PR objective and the AI-generated summary. It improves clarity by using a more specific type name.


19-19: LGTM!

Updating the constructor function newObjectIndexer to accept schema.StateObjectType instead of schema.ObjectType is necessary to match the updated type of the typ field in the objectIndexer struct. This change is consistent with the PR objective and the AI-generated summary.

schema/testing/fmt.go (1)

14-14: LGTM! The change is a breaking change but is necessary and consistent with the PR objective.

I can confirm that:

  • The change in the function signature from schema.ObjectType to schema.StateObjectType is consistent with the PR objective of renaming ObjectType to StateObjectType to reduce confusion and improve clarity.
  • The change is a breaking change, as it affects the public API of the function.
  • The change is necessary to ensure that the function works with the new StateObjectType type.
  • The function logic remains unchanged, and the change is limited to the parameter type.
  • The change does not introduce any new issues or bugs.
schema/view/object.go (4)

6-9: LGTM!

The comment provides clear guidelines on the usage of StateObjectUpdate and handling of ValueUpdates. It helps in understanding the expected behavior.


13-13: Looks good!

The change in the return type of ObjectType method to schema.StateObjectType aligns with the overall renaming effort to provide more specific types related to state management.


18-18: Looks good!

The change in the return type of GetObject method to schema.StateObjectUpdate is consistent with the renaming effort to provide more specific types related to state management.


23-23: Looks good!

The change in the function parameter type of AllState method to schema.StateObjectUpdate is consistent with the renaming effort to provide more specific types related to state management.

schema/testing/fmt_test.go (5)

11-11: LGTM!

The change from schema.ObjectType to schema.StateObjectType aligns with the PR objective and is consistent with the AI-generated summary. It does not introduce any issues in the test case.


Line range hint 16-22: Looks good!

The objectType field has been correctly updated to use schema.StateObjectType, consistent with the change at line 11. The test case structure remains intact.


26-30: Looks good!

The objectType field has been correctly updated to use schema.StateObjectType, consistent with the change at line 11. The test case structure remains intact.


Line range hint 34-43: Looks good!

The objectType field has been correctly updated to use schema.StateObjectType, consistent with the change at line 11. The test case structure remains intact.


Line range hint 9-64: Test coverage looks sufficient.

The TestObjectKeyString function provides good test coverage for the ObjectKeyString function. It tests the function with different object types and key values, covering important scenarios and edge cases. The assertions ensure that the function produces the expected output for each test case.

indexer/postgres/module.go (1)

Line range hint 43-51: LGTM!

The change from ObjectTypes to StateObjectTypes aligns with the PR objective of improving clarity by using a more specific type name. The table creation logic remains unchanged and error handling is still in place.

The code segment adheres to the Uber Go Style Guide.

schema/decoder.go (2)

16-18: LGTM!

The changes to the KVDecoder function signature and associated comment accurately reflect the transition to state-specific object updates. The code segment follows the Uber Golang style guide.


27-27: LGTM!

The change to the KVDecoder type alias accurately reflects the transition to state-specific object updates. The code segment follows the Uber Golang style guide.

indexer/postgres/internal/testdata/example_schema.go (4)

7-7: LGTM!

The change from schema.ObjectType to schema.StateObjectType for AllKindsObject aligns with the PR objective of improving clarity by using more specific state-related types.


10-10: Looks good!

The initialization of AllKindsObject has been correctly updated to match the type change to schema.StateObjectType.


48-48: LGTM!

The change from schema.ObjectType to schema.StateObjectType for SingletonObject aligns with the PR objective of improving clarity by using more specific state-related types.


68-68: Looks good!

The change from schema.ObjectType to schema.StateObjectType for VoteObject aligns with the PR objective of improving clarity by using more specific state-related types.

schema/type.go (6)

4-4: LGTM!

The comment change is consistent with the PR objective and the code changes made in the rest of the file.


25-26: LGTM!

The new LookupStateObjectType method is consistent with the existing LookupEnumType method and aligns with the PR objective.


39-41: LGTM!

The new StateObjectTypes method is consistent with the existing EnumTypes method and aligns with the PR objective.


65-66: LGTM!

The LookupStateObjectType method change is consistent with the changes made to the TypeSet interface and aligns with the PR objective.


73-73: LGTM!

The StateObjectTypes method change is consistent with the changes made to the TypeSet interface and aligns with the PR objective.


Line range hint 1-76: Conforms to the Uber Golang style guide.

The file follows the Uber Golang style guide, using the correct naming conventions, formatting, and indentation.

indexer/postgres/create_table_test.go (2)

82-84: LGTM!

The function signature update is consistent with the PR objective. No other changes or additional tests are required for this function.


Line range hint 86-96: Looks good, but consider adding more test cases if needed.

The function signature update is consistent with the PR objective. The existing example functions seem to cover the SQL generation logic sufficiently. However, consider adding more example functions if there are specific StateObjectTypes that warrant additional test cases to maintain adequate coverage.

indexer/postgres/view.go (3)

103-103: LGTM!

The function change aligns with the PR objective of renaming ObjectType to StateObjectType to improve clarity. The implementation is correct.


107-107: Verify the implementation of tm.get.

The function change aligns with the PR objective of using more specific state-related types. The implementation looks good, assuming that tm.get has been updated to return schema.StateObjectUpdate.

Please ensure that the implementation of tm.get has been updated to return schema.StateObjectUpdate.


111-111: Verify the implementation of tm.readRow.

The function change aligns with the PR objective of using more specific state-related types. The implementation looks good, assuming that tm.readRow has been updated to return schema.StateObjectUpdate.

Please ensure that the implementation of tm.readRow has been updated to return schema.StateObjectUpdate.

schema/testing/statesim/module.go (6)

19-19: LGTM!

The change in the type of updateGen field from *rapid.Generator[schema.ObjectUpdate] to *rapid.Generator[schema.StateObjectUpdate] aligns with the PR objective of transitioning to more specific state-related types. This improves clarity and specificity in the module's state management.


27-27: Looks good!

Updating the NewModule function to use moduleSchema.StateObjectTypes instead of moduleSchema.ObjectTypes ensures that the module is specifically working with state object types. This change enhances the module's alignment with state management and improves code clarity.


Line range hint 36-41: Approved!

Modifying the generator for updates to generate schema.StateObjectUpdate instances instead of schema.ObjectUpdate is a correct implementation that aligns with the transition to more specific state-related types. This change ensures consistency in handling state object updates throughout the module's functionality.


52-52: Change looks good!

Updating the ApplyUpdate method to accept schema.StateObjectUpdate instead of schema.ObjectUpdate reflects the shift towards handling more specific state-related updates. This change ensures that the module consistently works with state object updates, improving code clarity and maintainability.


63-63: Approved!

Updating the return type of the UpdateGen method to *rapid.Generator[schema.StateObjectUpdate] ensures consistency with the transition to more specific state-related types. This change maintains coherence throughout the module and improves code clarity by accurately reflecting the type of updates being generated.


Line range hint 1-97: Conforms to the Uber Golang Style Guide.

A thorough review of the code in this file confirms that it adheres to the guidelines and best practices outlined in the Uber Golang Style Guide. The naming conventions, formatting, and code structure are consistent with the recommendations provided in the guide. No deviations or style issues were identified.

schema/decoding/resolver_test.go (4)

13-13: LGTM!

The change from schema.ObjectType to schema.StateObjectType aligns with the PR objective and maintains the existing schema structure. The code segment adheres to the Uber Golang style guide.


25-25: Looks good!

The transition from schema.ObjectType to schema.StateObjectType is consistent with the changes made in modA and aligns with the PR objective. The code segment follows the Uber Golang style guide.


48-48: Looks good to me!

The type assertion update from schema.ObjectType to schema.StateObjectType correctly reflects the changes made in the ModuleCodec methods. The test logic is preserved, and the code segment adheres to the Uber Golang style guide.


Line range hint 1-144: Unit test coverage looks sufficient.

The unit tests in this file provide adequate coverage for the ModuleSetDecoderResolver functionality, considering the changes made in the pull request. The tests verify the behavior of the code with the updated StateObjectType and ensure that the essential functionality, such as iteration, decoder lookup, and error handling, works as expected. The test cases cover different scenarios and check for the presence of expected object types within the module schemas.

Based on the provided tests, the code coverage appears to be sufficient for the changes associated with this pull request.

schema/testing/object.go (3)

Line range hint 10-44: LGTM!

The renaming of ObjectTypeGen to StateObjectTypeGen and the change in return type from schema.ObjectType to schema.StateObjectType align well with the PR objective to transition to more specific state-related types. The internal logic remains sound, correctly filtering out duplicate field names. The function is well-structured and follows the Uber Go Style Guide.


58-60: Looks good!

The renaming of ObjectInsertGen to StateObjectInsertGen and the update of the parameter type from schema.ObjectType to schema.StateObjectType are in line with the PR objective. The function correctly calls StateObjectUpdateGen with the provided objectType and typeSet, and nil for the state parameter. The implementation follows the Uber Go Style Guide.


Line range hint 63-103: Excellent work!

The renaming of ObjectUpdateGen to StateObjectUpdateGen and the update of the parameter and return types to use schema.StateObjectType and schema.StateObjectUpdate are consistent with the PR objective. The internal logic has been correctly adapted to accommodate the new state-specific types, ensuring that the function generates valid updates and filters out keys that exist in the state. The implementation is well-structured and adheres to the Uber Go Style Guide.

schema/testing/statesim/app.go (1)

45-45: LGTM! The type change aligns with the refactoring goal.

The change from schema.ObjectUpdate to schema.StateObjectUpdate for the updates slice is consistent with the overall refactoring goal of transitioning to more specific state-related types. This improves clarity and reduces confusion in the context of state management.

Please ensure that the schema.StateObjectUpdate type is defined and used consistently across the codebase. You can use the following command to search for its usage:

Verification successful

Verification Successful: Consistent Usage of schema.StateObjectUpdate Across the Codebase

The schema.StateObjectUpdate type is consistently used across various files in the codebase, aligning with the refactoring goal of transitioning to more specific state-related types. This confirms the change improves clarity and reduces confusion in state management contexts. No issues were found.

  • Files with usage include schema/view/object.go, schema/testing/object.go, schema/testing/statesim/module.go, schema/testing/statesim/object_coll.go, schema/testing/statesim/app.go, schema/testing/appdatasim/app_data.go, schema/decoding/decoding_test.go, schema/appdata/data.go, and indexer/postgres/view.go.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `schema.StateObjectUpdate` type across the codebase.

# Test: Search for the type usage. Expect: Consistent usage across the codebase.
rg --type go $'schema.StateObjectUpdate'

Length of output: 4395

schema/testing/example_schema.go (8)

Line range hint 14-28: LGTM!

The change from schema.ObjectType to schema.StateObjectType for the "Singleton" object is consistent with the PR objective. The key and value fields seem appropriate.


Line range hint 28-47: Looks good!

The transition to schema.StateObjectType for the "Simple" object aligns with the PR's refactoring goal. The string key field and int32/bytes value fields are suitable.


Line range hint 47-60: Looks good to me!

Changing to schema.StateObjectType for "TwoKeys" matches the PR's intent. Using a string and int32 as key fields is valid, and omitting value fields is acceptable for objects only needing unique keys.


Line range hint 60-83: Looks good!

Updating "ThreeKeys" to use schema.StateObjectType is in line with the PR's refactoring goal. The combination of string, int32, and uint64 key fields, along with an int32 value field, illustrates the flexibility in defining object schemas.


Line range hint 83-110: LGTM!

Refactoring "ManyValues" to schema.StateObjectType follows the PR's goal. The string key field and diverse set of value fields (int32, bytes, float64, uint64) demonstrate the range of data an object can hold.


Line range hint 110-129: Looks good!

Changing "RetainDeletions" to schema.StateObjectType aligns with the PR's refactoring aim. The string key and int32/bytes value fields are appropriate, and enabling RetainDeletions could be beneficial for auditing or recovery scenarios.


Line range hint 144-169: LGTM!

Updating mkTestObjectType to return schema.StateObjectType instead of schema.ObjectType is consistent with the PR's refactoring goal. The function's logic for generating object types based on the provided kind remains appropriately unchanged.


Line range hint 1-176: Overall assessment: LGTM!

The changes in this file consistently refactor schema.ObjectType to schema.StateObjectType across various example object definitions and the mkTestObjectType function. The updates align with the PR's goal of renaming the type to better convey its state-related purpose. The example objects showcase a range of valid configurations, including different key and value field types, multiple keys, and the RetainDeletions property. No issues or inconsistencies were found in the changes.

schema/state_object.go (6)

5-6: LGTM!

The renaming of ObjectType to StateObjectType and the updated comment improve clarity and align with the PR objective.


33-33: LGTM!

The TypeName method has been correctly updated to use the new StateObjectType receiver type.


37-37: LGTM!

The isType method has been correctly updated to use the new StateObjectType receiver type.


40-40: LGTM!

The Validate method has been correctly updated to use the new StateObjectType receiver type.


85-85: LGTM!

The ValidateObjectUpdate method has been correctly updated to use the new StateObjectType receiver type and StateObjectUpdate parameter type, maintaining consistency with the new naming convention.


Line range hint 1-100: Code style looks good!

The code in this file conforms to the Uber Golang style guide. No deviations were found.

schema/diff/state_object_diff.go (5)

5-8: LGTM!

The renaming of ObjectTypeDiff to StateObjectTypeDiff improves clarity and aligns with the PR objective. The struct fields remain unchanged, preserving the functionality.


42-43: LGTM!

The changes to the compareObjectType function signature and return type are consistent with the renaming of ObjectTypeDiff to StateObjectTypeDiff. The function logic remains unchanged, preserving the functionality.


104-104: LGTM!

The change to the Empty method to operate on the StateObjectTypeDiff type is consistent with the renaming. The method logic remains unchanged, preserving the functionality.


110-110: LGTM!

The change to the HasCompatibleChanges method to operate on the StateObjectTypeDiff type is consistent with the renaming. The method logic remains unchanged, preserving the functionality.


Line range hint 1-141: Code style looks good!

The code in this file conforms to the Uber Golang style guide. It is well-structured, readable, and follows consistent naming conventions. The comments are clear and concise.

schema/testing/statesim/object_coll.go (12)

13-13: LGTM!

The comment update accurately reflects the transition to state-specific object types, aligning with the PR objectives.


16-16: LGTM!

The type change from schema.ObjectType to schema.StateObjectType for the objectType field aligns with the PR objectives of enhancing type specificity for state management.


18-18: LGTM!

Updating the objects field to use schema.StateObjectUpdate as the value type is in line with the PR objectives of transitioning to state-specific types for enhanced clarity in state management.


19-19: LGTM!

Modifying the updateGen field to generate schema.StateObjectUpdate instead of schema.ObjectUpdate aligns with the PR objectives of transitioning to state-specific types for enhanced clarity in state management.


24-24: LGTM!

Updating the NewObjectCollection function signature to accept schema.StateObjectType instead of schema.ObjectType is in line with the PR objectives of transitioning to state-specific types for enhanced clarity in state management.


25-25: LGTM!

Modifying the objects variable to use schema.StateObjectUpdate as the value type aligns with the PR objectives of transitioning to state-specific types for enhanced clarity in state management.


26-26: LGTM!

Updating the updateGen variable to use schematesting.StateObjectUpdateGen instead of schematesting.ObjectUpdateGen is in line with the PR objectives of transitioning to state-specific types for enhanced clarity in state management.


43-43: LGTM!

Modifying the ApplyUpdate method signature to accept schema.StateObjectUpdate instead of schema.ObjectUpdate aligns with the PR objectives of transitioning to state-specific types for enhanced clarity in state management.


109-109: LGTM!

Updating the UpdateGen method signature to return a generator of schema.StateObjectUpdate instead of schema.ObjectUpdate is in line with the PR objectives of transitioning to state-specific types for enhanced clarity in state management.


115-115: LGTM!

Modifying the AllState method signature to accept a function that takes schema.StateObjectUpdate instead of schema.ObjectUpdate aligns with the PR objectives of transitioning to state-specific types for enhanced clarity in state management.


116-116: LGTM!

Updating the objects.Scan method callback to use schema.StateObjectUpdate instead of schema.ObjectUpdate is in line with the PR objectives of transitioning to state-specific types for enhanced clarity in state management.


121-123: LGTM!

The following changes align with the PR objectives of transitioning to state-specific types for enhanced clarity in state management:

  • Updating the GetObject method signature to return schema.StateObjectUpdate instead of schema.ObjectUpdate.
  • Modifying the ObjectType method signature to return schema.StateObjectType instead of schema.ObjectType.
  • Updating the comments for the GetObject method to mention "StateObjectUpdate".

Also applies to: 129-129

schema/diff/diff.go (6)

7-8: LGTM!

The renaming of AddedObjectTypes to AddedStateObjectTypes and the corresponding type change to []schema.StateObjectType align well with the PR objective to enhance clarity by using more specific state-related types.


10-11: LGTM!

The renaming of ChangedObjectTypes to ChangedStateObjectTypes and the corresponding type change to []StateObjectTypeDiff align well with the PR objective to enhance clarity by using more specific state-related types.


13-14: LGTM!

The renaming of RemovedObjectTypes to RemovedStateObjectTypes and the corresponding type change to []schema.StateObjectType align well with the PR objective to enhance clarity by using more specific state-related types.


44-52: LGTM!

The changes in this code segment, such as replacing oldSchema.ObjectTypes with oldSchema.StateObjectTypes, newSchema.LookupObjectType with newSchema.LookupStateObjectType, diff.RemovedObjectTypes with diff.RemovedStateObjectTypes, and diff.ChangedObjectTypes with diff.ChangedStateObjectTypes, align well with the renaming of object types to state object types. The lookup methods and diff fields have been updated consistently.


57-60: LGTM!

The changes in this code segment, such as replacing newSchema.ObjectTypes with newSchema.StateObjectTypes, oldSchema.LookupObjectType with oldSchema.LookupStateObjectType, and diff.AddedObjectTypes with diff.AddedStateObjectTypes, align well with the renaming of object types to state object types. The lookup methods and diff fields have been updated consistently.


Line range hint 90-105: LGTM!

The changes in this code segment, such as replacing m.AddedObjectTypes with m.AddedStateObjectTypes, m.ChangedObjectTypes with m.ChangedStateObjectTypes, and m.RemovedObjectTypes with m.RemovedStateObjectTypes, align well with the renaming of object types to state object types. The Empty and HasCompatibleChanges methods have been updated consistently.

schema/appdata/data.go (1)

134-134: LGTM! Verify the impact of this change.

The change from []schema.ObjectUpdate to []schema.StateObjectUpdate for the Updates field in ObjectUpdateData struct aligns well with the stated PR objectives of enhancing type specificity and clarity in handling state management. The code change itself looks good.

However, since this is a breaking change, please ensure that all code locations that use the ObjectUpdateData struct have been updated to handle the new []schema.StateObjectUpdate type for the Updates field. Run the following script to verify:

Verification successful

Verification Successful: Change to ObjectUpdateData Struct Propagated Correctly

The transition from []schema.ObjectUpdate to []schema.StateObjectUpdate for the Updates field in the ObjectUpdateData struct has been successfully propagated across the codebase. All usages of ObjectUpdateData reflect this change, and no issues were found with unrelated occurrences of schema.ObjectUpdate.

  • The ObjectUpdateData struct is consistently used with StateObjectUpdate in various files.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all usages of `ObjectUpdateData` struct are updated to handle the new type.

# Test 1: Search for `ObjectUpdateData` struct usages. 
# Expect: Only occurrences with `StateObjectUpdate` type for `Updates` field.
rg --type go $'ObjectUpdateData' -A 5

# Test 2: Search for `schema.ObjectUpdate` usages. 
# Expect: Only occurrences unrelated to `ObjectUpdateData` struct.
rg --type go $'schema\.ObjectUpdate'

Length of output: 9805

schema/module_schema.go (6)

60-66: LGTM!

The changes to the ValidateObjectUpdate function signature and internal type handling align with the broader refactoring to transition from using general object types to more specific state-related types. The modifications ensure that the validation logic aligns with the new type definitions.


94-101: LGTM!

The renaming of the LookupObjectType method to LookupStateObjectType and the corresponding changes to the return type align with the broader refactoring to transition from ObjectType to StateObjectType. The modifications are consistent with the goal of distinguishing between different categories of object types.


122-124: LGTM!

The renaming of the ObjectTypes method to StateObjectTypes and the corresponding changes to the parameter type of the callback function align with the broader refactoring to transition from ObjectType to StateObjectType. The modifications are consistent with the goal of distinguishing between different categories of object types.


144-145: LGTM!

The changes to the ObjectTypes field in the moduleSchemaJson struct to expect a slice of StateObjectType instead of ObjectType align with the broader refactoring to transition from using general object types to more specific state-related types. The modifications ensure that the JSON marshaling structure reflects the new type definitions.


154-154: LGTM!

The changes to the MarshalJSON method to use StateObjectTypes instead of ObjectTypes align with the broader refactoring to transition from using general object types to more specific state-related types. The modifications ensure that the JSON marshaling logic reflects the new type definitions.


60-66: Conforms to the Uber Golang style guide.

The code segments adhere to the Uber Golang style guide in terms of naming conventions, function signatures, and struct field definitions. No deviations from the style guide are observed in the provided code segments.

Also applies to: 94-101, 122-124, 144-145, 154-154

schema/state_object_test.go (16)

Line range hint 8-16: LGTM!

The change from ObjectType to StateObjectType enhances clarity and the field definition is valid.


Line range hint 18-29: Looks good!

The change to StateObjectType with valid key field definitions is approved.


Line range hint 31-43: Approved.

The change to StateObjectType with valid value field definitions looks good.


Line range hint 45-60: Change approved.

The transition to StateObjectType with valid key and value field definitions is good.


64-64: LGTM.

Updating the objectType field to StateObjectType in the test struct is consistent with the new type definitions.


Line range hint 74-83: Test case looks good.

Using StateObjectType in the "empty object type name" test case is consistent with the new type definition. It's important to validate the behavior when the object type name is empty.


Line range hint 87-97: Approved.

Using StateObjectType in the "invalid key field" test case is consistent with the new type definition. Validating the behavior when a key field has an invalid definition is crucial.


Line range hint 100-109: Change looks good.

Using StateObjectType in the "invalid value field" test case is consistent with the new type definition. Testing the behavior when a value field has an invalid definition is important.


112-114: Test case approved.

Using StateObjectType in the "no fields" test case is consistent with the new type definition. Validating the behavior when an object type has no fields defined is a good edge case to cover.


Line range hint 117-134: LGTM.

Using StateObjectType in the "duplicate field" test case is consistent with the new type definition. Testing the behavior when an object type has a duplicate field name is crucial for maintaining data integrity.


Line range hint 153-164: Test case looks good.

Using StateObjectType in the "nullable key field" test case is consistent with the new type definition. Validating that key fields cannot be nullable is crucial for maintaining data integrity and uniqueness.


Line range hint 167-177: Change approved.

Using StateObjectType in the "float32 key field" test case is consistent with the new type definition. Testing that float32 is an invalid key field type is important for maintaining data integrity and ensuring key fields support uniqueness and equality comparison.


Line range hint 180-190: LGTM.

Using StateObjectType in the "float64 key field" test case is consistent with the new type definition. Validating that float64 is an invalid key field type, similar to float32, is important for maintaining data integrity and ensuring key fields support uniqueness and equality comparison.


Line range hint 193-203: Test case approved.

Using StateObjectType in the "json key field" test case is consistent with the new type definition. Testing that JSON is an invalid key field type is crucial, as complex types like JSON are not suitable for keys and don't support efficient equality comparison.


225-225: Changes look good.

Updating the objectType fields to StateObjectType and object fields to StateObjectUpdate in the TestObjectType_ValidateObjectUpdate test cases is consistent with the transition to state-specific types. The test cases cover important scenarios for validating object updates, ensuring the correctness of the ValidateObjectUpdate method.

Also applies to: 226-226, 232-236, 241-245, 250-254, 259-264


Line range hint 1-288: Overall, the changes in schema/state_object_test.go look great!

The transition from ObjectType to StateObjectType and ObjectUpdate to StateObjectUpdate aligns perfectly with the PR objective of enhancing clarity and reducing confusion. The changes are consistently applied throughout the file, and the test cases cover a wide range of scenarios, ensuring the correctness of the Validate and ValidateObjectUpdate methods.

The test cases validate important aspects such as empty object type

indexer/postgres/select.go (2)

Line range hint 150-214: Verify the function usage.

The changes to the readRow function look good and align with the PR objective of renaming ObjectType to StateObjectType. However, ensure that the return type change is thoroughly tested to confirm that it behaves as expected, and that all function calls to readRow have been updated to handle the new return type.

Run the following script to verify the function usage:

Verification successful

The usage of the readRow function in view.go and select.go appears to handle the new return type correctly. However, to ensure thorough verification, we need to check the calling function in select.go to confirm it expects the new return type. Let's run a script to identify the calling function and verify its handling of the return type.


Let's refine our approach to identify the function in select.go that calls readRow and verify its handling of the return type.


To ensure the calling function in select.go correctly handles the new return type from readRow, let's capture the full function definition.


The function in select.go that calls readRow correctly handles the new return type of schema.StateObjectUpdate, bool, and error. The changes are consistent with the PR objectives and do not introduce any issues.

  • The function usage in select.go is correctly aligned with the new return type of readRow.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `readRow` handle the new return type.

# Test: Search for the function usage. Expect: No type mismatches.
rg --type go -A 5 $'readRow\('

Length of output: 998


Script:

#!/bin/bash
# Description: Verify the calling function of `readRow` in `select.go` to ensure it expects the new return type.

# Find the function that calls `readRow` in `select.go` and check its return type handling.
rg --type go -A 10 'return tm.readRow(row)' indexer/postgres/select.go

Length of output: 72


Script:

#!/bin/bash
# Description: Search for the function definition in `select.go` that contains the `readRow` call.

# Find the function definition that includes the `readRow` call.
rg --type go -A 20 'func ' indexer/postgres/select.go | rg -B 5 'readRow'

Length of output: 492


Script:

#!/bin/bash
# Description: Capture the full function definition in `select.go` that includes the `readRow` call.

# Find the full function definition that includes the `readRow` call.
rg --type go -A 30 'func ' indexer/postgres/select.go | rg -B 10 -A 20 'readRow'

Length of output: 1498


Line range hint 73-87: Verify the function usage.

The changes to the get function look good and align with the PR objective of renaming ObjectType to StateObjectType. However, ensure that the return type change is thoroughly tested to confirm that it behaves as expected, and that all function calls to get have been updated to handle the new return type.

Run the following script to verify the function usage:

Verification successful

Return Type Change Verified for get Function

The return type change from schema.ObjectUpdate to schema.StateObjectUpdate in the get function is correctly handled in the codebase. The GetObject method, which exposes this functionality, is used in a manner that appropriately checks for errors and object existence.

  • Verified usage in schema/testing/statesim/object_coll_diff.go.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `get` handle the new return type.

# Test: Search for the function usage. Expect: No type mismatches.
rg --type go -A 5 $'get\('

Length of output: 32402


Script:

#!/bin/bash
# Description: Verify all function calls to `objectView.get` handle the new return type.

# Test: Search for the function usage. Expect: No type mismatches.
rg --type go -A 5 $'get('

Length of output: 85


Script:

#!/bin/bash
# Description: Verify all function calls to `objectView.get` handle the new return type.

# Test: Search for the function usage. Expect: No type mismatches.
rg --type go -A 5 $'objectView.get\\('

Length of output: 38


Script:

#!/bin/bash
# Description: Search for any usage of `objectView` to identify potential indirect calls to the `get` method.

# Test: Search for `objectView` usage. Expect: Identify contexts where `get` might be called indirectly.
rg --type go -A 5 $'objectView'

Length of output: 1815


Script:

#!/bin/bash
# Description: Verify all function calls to `objectView.GetObject` handle the new return type.

# Test: Search for the function usage. Expect: No type mismatches.
rg --type go -A 5 $'GetObject\\('

Length of output: 1934

schema/module_schema_test.go (16)

Line range hint 18-25: LGTM!

The renaming of ObjectType to StateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


Line range hint 33-40: LGTM!

The renaming of ObjectType to StateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


Line range hint 48-57: LGTM!

The renaming of ObjectType to StateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


88-89: LGTM!

The renaming of objectUpdate parameter type from ObjectUpdate to StateObjectUpdate in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


Line range hint 94-102: LGTM!

The renaming of ObjectType to StateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


104-107: LGTM!

The renaming of ObjectUpdate to StateObjectUpdate in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


Line range hint 113-121: LGTM!

The renaming of ObjectType to StateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


123-126: LGTM!

The renaming of ObjectUpdate to StateObjectUpdate in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


Line range hint 131-143: LGTM!

The renaming of ObjectType to StateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


146-149: LGTM!

The renaming of ObjectUpdate to StateObjectUpdate in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


Line range hint 180-187: LGTM!

The renaming of ObjectType to StateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


190-191: LGTM!

The renaming of LookupObjectType to LookupStateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


Line range hint 203-211: LGTM!

The renaming of ObjectType to StateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


Line range hint 213-221: LGTM!

The renaming of ObjectType to StateObjectType in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


265-268: LGTM!

The renaming of ObjectTypes to StateObjectTypes in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.


277-280: LGTM!

The renaming of ObjectTypes to StateObjectTypes in this test case aligns with the overall refactoring goal of transitioning to more specific state-related types. The change looks good.

schema/diff/state_object_diff_test.go (2)

Line range hint 10-145: LGTM!

The test cases have been comprehensively updated to cover the changes in the compareObjectType function. The test cases cover all the important scenarios and edge cases, and use the reflect.DeepEqual function to compare the expected and actual values, which is a good practice. The test cases also verify the HasCompatibleChanges function, which is important to ensure that the changes are compatible.


Line range hint 147-249: LGTM!

The test cases comprehensively cover the compareFields function. The test cases cover all the important scenarios and edge cases, and use the reflect.DeepEqual function to compare the expected and actual values, which is a good practice.

schema/diff/diff_test.go (14)

Line range hint 21-29: LGTM!

The test case correctly compares two identical schema.StateObjectType instances and expects an empty diff. The transition from ObjectType to StateObjectType is consistent.


Line range hint 36-47: LGTM!

The test case correctly compares an empty schema with a schema containing one schema.StateObjectType and expects a diff with one added state object type. The transition from ObjectType to StateObjectType is consistent.


Line range hint 52-64: LGTM!

The test case correctly compares a schema containing one schema.StateObjectType with an empty schema and expects a diff with one removed state object type. The transition from ObjectType to StateObjectType is consistent.


Line range hint 69-89: LGTM!

The test case correctly compares two schema.StateObjectType instances with different key fields and expects a diff with one changed state object type containing the key fields diff. The transition from ObjectType to StateObjectType is consistent.


Line range hint 93-112: LGTM!

The test case correctly compares two schema.StateObjectType instances with different value fields and expects a diff with one changed state object type containing the value fields diff. The transition from ObjectType to StateObjectType is consistent.


Line range hint 116-135: LGTM!

The test case correctly compares two schema.StateObjectType instances with different value fields and expects a diff with one changed state object type containing the value fields diff. The transition from ObjectType to StateObjectType is consistent.


Line range hint 139-158: LGTM!

The test case correctly compares two schema.StateObjectType instances with reordered key fields and expects a diff with one changed state object type containing the old and new order of key fields. The transition from ObjectType to StateObjectType is consistent.


Line range hint 162-198: LGTM!

The test case correctly compares two schemas with different state object types and enum types. It expects a diff with one changed state object type containing the value fields diff and one added enum type. The transition from ObjectType to StateObjectType is consistent.


Line range hint 204-237: LGTM!

The test case correctly compares two schemas with different state object types and enum types. It expects a diff with one changed state object type containing the value fields diff and one removed enum type. The transition from ObjectType to StateObjectType is consistent.


Line range hint 240-257: LGTM!

The test case correctly compares two schemas with different enum types and expects a diff with one changed enum type containing the added enum value.


Line range hint 260-277: LGTM!

The test case correctly compares two schemas with different enum types and expects a diff with one changed enum type containing the removed enum value.


Line range hint 280-312: LGTM!

The test case correctly compares two schemas with switched object type and enum type names. It expects a diff with one removed and one added state object type, and one removed and one added enum type. The transition from ObjectType to StateObjectType is consistent.


Line range hint 1-345: The unit test code provides sufficient coverage.

The test cases cover various scenarios such as no change, object type added/removed/changed, enum type added/removed, and enum value added/removed. They also cover scenarios with compatible and incompatible changes. This provides a comprehensive coverage for the CompareModuleSchemas function and the related diff types.


Line range hint 1-345: The code adheres to the Uber Go Style Guide.

The code follows the basic Go style conventions such as using camelCase for variable and function names, and using uppercase for exported names. It uses proper indentation and spacing, and uses meaningful and concise names for variables and functions. There are no major deviations from the Uber Go Style Guide.

schema/decoding/decoding_test.go (6)

Line range hint 32-50: LGTM!

The renaming of expectedBank and expectedOne to use schema.StateObjectUpdate aligns with the broader refactoring and is necessary for the test case to pass.

Also applies to: 59-61


87-89: LGTM!

The renaming of expectedOne to use schema.StateObjectUpdate aligns with the broader refactoring and is necessary for the test case to pass.


Line range hint 111-125: LGTM!

The renaming of expected and expectedOne to use schema.StateObjectUpdate aligns with the broader refactoring and is necessary for the test case to pass.

Also applies to: 133-135


160-162: LGTM!

The renaming of expectedOne to use schema.StateObjectUpdate aligns with the broader refactoring and is necessary for the test case to pass.


Line range hint 403-426: LGTM!

The renaming of the KVDecoder return type to use schema.StateObjectUpdate aligns with the broader refactoring and is necessary for the module codec to be compatible with the refactored codebase.


454-461: LGTM!

The renaming of the KVDecoder return type to use schema.StateObjectUpdate aligns with the broader refactoring and is necessary for the module codec to be compatible with the refactored codebase.

@@ -119,7 +119,7 @@ func (a *Simulator) BlockDataGenN(minUpdatesPerBlock, maxUpdatesPerBlock int) *r
})
}

func (a *Simulator) formatUpdateKey(moduleName string, update schema.ObjectUpdate) string {
func (a *Simulator) formatUpdateKey(moduleName string, update schema.StateObjectUpdate) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, but consider returning the error.

The change in the function signature from schema.ObjectUpdate to schema.StateObjectUpdate aligns well with the PR objective to transition to more specific state-related types. The logic of the function remains unchanged, so this is a non-breaking change.

However, the function uses panic to handle errors from GetModule and GetObjectCollection, which is not recommended in a library intended for reuse. Consider returning the error instead to allow the caller to handle it appropriately.

Apply this diff to return the error:

-func (a *Simulator) formatUpdateKey(moduleName string, update schema.StateObjectUpdate) string {
+func (a *Simulator) formatUpdateKey(moduleName string, update schema.StateObjectUpdate) (string, error) {
 	mod, err := a.state.GetModule(moduleName)
 	if err != nil {
-		panic(err)
+		return "", err
 	}
 
 	objColl, err := mod.GetObjectCollection(update.TypeName)
 	if err != nil {
-		panic(err)
+		return "", err
 	}
 
 	ks := fmt.Sprintf("%s:%s:%s", moduleName, update.TypeName, schematesting.ObjectKeyString(objColl.ObjectType(), update.Key))
-	return ks
+	return ks, nil
 }

Then, update the callers of formatUpdateKey to handle the returned error.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (a *Simulator) formatUpdateKey(moduleName string, update schema.StateObjectUpdate) string {
func (a *Simulator) formatUpdateKey(moduleName string, update schema.StateObjectUpdate) (string, error) {
mod, err := a.state.GetModule(moduleName)
if err != nil {
return "", err
}
objColl, err := mod.GetObjectCollection(update.TypeName)
if err != nil {
return "", err
}
ks := fmt.Sprintf("%s:%s:%s", moduleName, update.TypeName, schematesting.ObjectKeyString(objColl.ObjectType(), update.Key))
return ks, nil
}

Copy link
Contributor

@testinginprod testinginprod left a comment

Choose a reason for hiding this comment

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

nice!

@cool-develope
Copy link
Contributor

@aaronc , need to fix the lint issue

@tac0turtle tac0turtle added this pull request to the merge queue Sep 16, 2024
Merged via the queue into main with commit ae40e80 Sep 16, 2024
74 checks passed
@tac0turtle tac0turtle deleted the aaronc/schema-state-object-rename branch September 16, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants