Skip to content

Export defaultFormattingOptions. #27

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

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Oct 4, 2021

No point having a default when the user cannot access to override it! Without this, you cannot write code like

... = defaultFormattingOptions{ exportMode = ExportEach }

This was forgotten in PR #22 when extracting it from PR #17.

Similar to PR #23.

CC @bitonic

No point having a default when the user cannot access to override it!

This was forgotten in PR codedownio#22 when extracting it from PR codedownio#17.
nh2 added a commit to nh2/solga that referenced this pull request Oct 4, 2021
…ixes.

Our fixes: codedownio/aeson-typescript#17

The one that wasn't taken verbatim, which this commit adapts to:

Upstremed in: codedownio/aeson-typescript#22

We still have to use our fork to fix `defaultFormattingOptions` not being
exported:

codedownio/aeson-typescript#27
@thomasjm thomasjm merged commit 16d6c2a into codedownio:master Oct 4, 2021
@thomasjm
Copy link
Contributor

thomasjm commented Oct 4, 2021

Ah good point haha. Now that I look at it I wish I hadn't exported the actual FormattingOptions constructor. Thanks!

@nh2
Copy link
Contributor Author

nh2 commented Oct 4, 2021

@thomasjm I think exporting is always the right thing to do. If it's internal and has no API guarantees, put it into an .Internal module -- but please do export it, so that if there are some use cases that require new exports, people can do it without having to fork the code (and the operational efforts that this makes).

@thomasjm
Copy link
Contributor

thomasjm commented Oct 4, 2021

I mean I wish I had followed the pattern of exporting the default options only and not the constructor itself, like this. It's a great pattern for being able to extend the options without causing breaking changes and AFAIK it has no downsides.

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