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

Format JSON file contents to allow for easier editing #525

Merged
merged 12 commits into from
Sep 21, 2023

Conversation

kelfish
Copy link
Contributor

@kelfish kelfish commented Sep 18, 2023

Currently the JSON file contents are written on a single line. If this external config.json needs to be edited externally, it is very hard to do so. This change uses a 2 space tabbed format when utils.sync is called to ensure the JSON is formatted in a more friendly, readable & editable format in the file.

Copy link
Owner

@de-luca de-luca left a comment

Choose a reason for hiding this comment

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

Thanks for submitting changes.
I'd guard this feature behind some configuration so the change of storage has no impact without explicitly requesting so.
This could be done by adding a facultative option object in the Config constructor.
It could then be used in the decorator to run the sync function with the expected behaviour.

@kelfish
Copy link
Contributor Author

kelfish commented Sep 18, 2023

Yeah, good idea, I will add that... I am thinking an options arg for the factory and the Config constructor. e.g.

type ConfigOptions = {
  prettyJson?: {
    enabled: boolean;
    tabSize?: number;
  };
};

Also, do you mind if I add prettier as a devDependency so my formatting aligns with yours?

@de-luca
Copy link
Owner

de-luca commented Sep 19, 2023

Also, do you mind if I add prettier as a devDependency so my formatting aligns with yours?

Can you split that one in another PR so limit the diff to the corresponding changes?

@kelfish
Copy link
Contributor Author

kelfish commented Sep 20, 2023

@de-luca - would you mind checking this PR now please? I will do a separate one after this with prettier setup...

src/utils.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #525 (0bb1529) into master (5c73bd8) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #525   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines           79        87    +8     
  Branches        11        16    +5     
=========================================
+ Hits            79        87    +8     
Files Changed Coverage Δ
src/Config.ts 100.00% <100.00%> (ø)
src/factory.ts 100.00% <100.00%> (ø)
src/utils.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kelfish
Copy link
Contributor Author

kelfish commented Sep 21, 2023

I've removed the redundant options? check as options is always defined, it has a default if undefined is passed.

@de-luca

@de-luca de-luca merged commit d94af02 into de-luca:master Sep 21, 2023
@de-luca
Copy link
Owner

de-luca commented Sep 21, 2023

Thank you for the submission, Good job!

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