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

chore: Improve Governancecodebase #52

Merged
merged 6 commits into from
Jun 14, 2024

Conversation

TAdev0
Copy link
Contributor

@TAdev0 TAdev0 commented Jun 9, 2024

Hey @moodysalem , while reading the code of the Governance contracts, I decided to push this small PR without any functionality modification:

  • remove unnecessary (imported by default with prelude 2023_11) and unused imports
  • reorganize imports where needed
  • remove unnecessary pub(crate) in multiple places
  • increase consistency in comments (full sentence with point at the end when commenting interfaces, etc)
  • use array macro everywhere to instantiate new arrays
  • Correct a few typos

@TAdev0 TAdev0 changed the title chore: Improve codebase chore: Improve Governancecodebase Jun 9, 2024
@moodysalem
Copy link
Member

moodysalem commented Jun 13, 2024

Personally not a big fan of code changes that are purely stylistic and aren't enforced by scarb fmt, a lot of this isn't objectively more readable, and it was my stylistic preference to use {} for imports even if it's just one

That said cleaning up comments I can get behind

@TAdev0
Copy link
Contributor Author

TAdev0 commented Jun 13, 2024

just left comments improvements.
also left array macros as you use them in various places and there was just like 4 remaining instantiation with arrayTrait::new() .
same for line skipped, i did it where it was supposed to be done given the style of the rest (skip line before returning a value as an expression for example)

@moodysalem moodysalem merged commit 16f9c87 into EkuboProtocol:main Jun 14, 2024
1 check passed
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.

2 participants