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

[TECHDEBT] Tackle TODOs in shared/modules Issue #232 #260

Merged
merged 22 commits into from
Oct 12, 2022
Merged

Conversation

andrewnguyen22
Copy link
Contributor

Description

  • Tackle / recategorize all non-blocked TODOs in shared/modules

Techdebt week emphasizes code health. Particularly, removing all placeholder and hacky code.

The past few months, shared/modules have built up quite a few TODOs.

The PR should remove all non-blocked TODOs

  • Reduce cognitive load
  • Increase code health

Issue

Fixes #232

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Deprecated persMod.ResetContext() for -> persRWContext.ResetContext() for more appropriate encapsulation
  • Added ticks to CHANGELOG.md
  • Removed reference to Utility Mod's BigIntToString() and used internal BigIntToString()
  • Used proper TODO/INVESTIGATE/DISCUSS convention across package
  • Moved TxIndexer Package to Utility to properly encapsulate

Testing

  • make develop_test
  • LocalNet w/ all of the steps outlined in the README

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works

@andrewnguyen22 andrewnguyen22 added the code health Nice to have code improvement label Sep 30, 2022
@andrewnguyen22 andrewnguyen22 self-assigned this Sep 30, 2022
@andrewnguyen22 andrewnguyen22 requested a review from a team as a code owner September 30, 2022 20:03
shared/modules/types.go Outdated Show resolved Hide resolved
shared/test_artifacts/generator.go Outdated Show resolved Hide resolved
utility/indexer/doc/README.md Outdated Show resolved Hide resolved
utility/indexer/proto/transaction_indexer.proto Outdated Show resolved Hide resolved
utility/test/actor_test.go Outdated Show resolved Hide resolved
shared/modules/doc/README.md Outdated Show resolved Hide resolved
shared/modules/doc/README.md Outdated Show resolved Hide resolved
shared/modules/doc/README.md Outdated Show resolved Hide resolved
shared/modules/doc/README.md Outdated Show resolved Hide resolved
shared/modules/doc/README.md Outdated Show resolved Hide resolved
Andrew Nguyen and others added 16 commits October 5, 2022 09:00
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Left a couple extra comments but changes LGTM otherwise.

Please resolve conflicts with main.

@Olshansk
Copy link
Member

Olshansk commented Oct 6, 2022

Changes LGTM but I'd like to get #282 in first since it might catch unit test failures that may or may not have been introduced.

@andrewnguyen22
Copy link
Contributor Author

andrewnguyen22 commented Oct 10, 2022

@Olshansk all tests pass for me after #282 and merge with main

@andrewnguyen22 andrewnguyen22 merged commit 2c390c3 into main Oct 12, 2022
@andrewnguyen22 andrewnguyen22 deleted the issue-#232 branch October 12, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Nice to have code improvement
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[TECHDEBT] Tackle TODOs in shared/modules
2 participants