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

Document and cleanup of utility functions #1442

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Document and cleanup of utility functions #1442

merged 3 commits into from
Dec 13, 2022

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Dec 12, 2022

  • Add documentation to most functionality in the ./utils folder.
  • Remove some dead code.

@oliviertassinari oliviertassinari requested a deployment to utils-cleanup - toolpad-db PR #1442 December 12, 2022 10:54 — with Render Abandoned
@oliviertassinari oliviertassinari temporarily deployed to utils-cleanup - toolpad PR #1442 December 12, 2022 10:55 — with Render Destroyed
@Janpot Janpot added the core Infrastructure work going on behind the scenes label Dec 12, 2022
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Nice work, should be pretty useful, thanks a lot!

@@ -31,20 +39,29 @@ export function mapProperties<U, V>(
);
}

/**
Copy link
Member

@apedroferreira apedroferreira Dec 12, 2022

Choose a reason for hiding this comment

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

Doesn't hurt to have these descriptions, but i feel like a few (not all) of these utilities are almost self-explanatory and don't really need comments? So maybe we don't need to enforce that every utility method always has comments (it's fine to keep them now though), and it's ok to rely on self-explanatory method names?

Also have we decided definitively if we want to maintain our own utilities or rely on an external library (just for the typical stuff that for example lodash already does)? I'm fine with either (maybe slightly more in favor of using a library), but we should just find a consensus I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also have we decided definitively if we want to maintain our own utilities or rely on an external library (just for the typical stuff that for example lodash already does)? I'm fine with either (maybe slightly more in favor of using a library), but we should just find a consensus I guess.

We will just keep making informed decisions based on the use-case at hand, the available options, our experience, the maintenance cost required, etc... and not fall in the trap of taking on dogmas around this. Making the decision of using a library that does 85% of what you need may as well be detrimental to your overall codebase if the other 15% turn into a maintenance hell. Almost all of functions that are in the utils are there because of shortcomings in available options.

@Janpot Janpot mentioned this pull request Dec 12, 2022
@oliviertassinari oliviertassinari temporarily deployed to utils-cleanup - toolpad PR #1442 December 12, 2022 14:57 — with Render Destroyed
@Janpot Janpot merged commit 9f719fb into master Dec 13, 2022
@Janpot Janpot deleted the utils-cleanup branch December 13, 2022 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants