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

feat: implement config package #142

Merged
merged 1 commit into from
Jul 14, 2024
Merged

feat: implement config package #142

merged 1 commit into from
Jul 14, 2024

Conversation

PierreDemailly
Copy link
Contributor

@PierreDemailly PierreDemailly commented Jul 13, 2024

TODOs

  • Generics
  • More tests
  • Do we need async methods ? The package is full sync

I would appreciate a first review @fraxken

@PierreDemailly PierreDemailly requested a review from fraxken July 13, 2024 20:38
@PierreDemailly PierreDemailly marked this pull request as draft July 13, 2024 20:39
@fraxken
Copy link
Member

fraxken commented Jul 13, 2024

I would have re-implemented it with async and the old test suite (not perfect but it was doing the job). Synchronous API are not idiot but require a deeper refactor.

And yes Generics is welcome

@PierreDemailly
Copy link
Contributor Author

I've added some test and the coverage is now 97%

@fraxken WDYM by deeper refactor ? I'd like to implement both sync and async methods (read, readSync, writeOnDisk, writeOnDiskSync etc), do you think its better to skip sync and keep only async ?

@fraxken
Copy link
Member

fraxken commented Jul 14, 2024

@PierreDemailly Some API has been designed to be lazy with async. Also having both make things a lot more complicated for me in term of how things are abstract.

src/config/README.md Outdated Show resolved Hide resolved
src/config/README.md Outdated Show resolved Hide resolved
src/config/README.md Outdated Show resolved Hide resolved
src/config/src/utils.ts Outdated Show resolved Hide resolved
src/config/src/index.ts Outdated Show resolved Hide resolved
@fraxken
Copy link
Member

fraxken commented Jul 14, 2024

I would rename Config -> ConfigSync and save it into a separate files. And keep index to export.

@PierreDemailly PierreDemailly marked this pull request as ready for review July 14, 2024 10:21
@fraxken fraxken merged commit 6d495cb into main Jul 14, 2024
3 checks passed
@fraxken fraxken deleted the config branch July 14, 2024 13:31
@fraxken
Copy link
Member

fraxken commented Aug 12, 2024

@allcontributors please add @PierreDemailly for code, docs, maintenance

Copy link
Contributor

@fraxken

I've put up a pull request to add @PierreDemailly! 🎉

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