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

refactor: combine check-tsconfig with parse-tsconfig #413

Merged

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Aug 24, 2022

NOTE: this is built on top of #404 as it uses the context changes made there. As such, I've marked this PR as "Draft" until #404 is merged.
Rebased on top and marked as ready for review.

Summary

Combine the two tsconfig files into one as the split is not currently necessary and adds unnecessary complexity

Details

  • checkTsConfig is literally only used by parseTsConfig

    • I first just moved the function over, but it's a two-liner, so thought it made more sense to just in-line it
  • merge the unit tests as well

    • we already check the non-error case in parse-tsconfig.spec, so only add the error case
  • check-tsconfig was longer in the past and more files were being created then for separation, so this may have made more sense then, but now it is unnecessary

@agilgur5 agilgur5 added the kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs label Aug 24, 2022
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Aug 25, 2022

This is gonna merge conflict with #404 -- please merge that first as it's a complicated one that a bunch of other code depends on EDIT: nvm, see below EDIT2: more of a "silent" merge conflict due to variable renaming, so will build this on top of #404 after all and fix the renaming issue. Sorry for any notification churn!

@agilgur5 agilgur5 marked this pull request as draft August 25, 2022 02:39
@agilgur5 agilgur5 force-pushed the refactor-combine-tsconfig-handlers branch 2 times, most recently from 48b7df2 to f015c78 Compare August 25, 2022 02:44
@agilgur5 agilgur5 marked this pull request as ready for review August 25, 2022 02:45
@agilgur5 agilgur5 force-pushed the refactor-combine-tsconfig-handlers branch from f015c78 to f4535c1 Compare August 25, 2022 02:50
@agilgur5 agilgur5 marked this pull request as draft August 25, 2022 02:50
- `checkTsConfig` is literally only used by `parseTsConfig`
  - I first just moved the function over, but it's a two-liner, so thought it made more sense to just in-line it

- merge the unit tests as well
  - we already check the non-error case in `parse-tsconfig.spec`, so only add the error case

- `check-tsconfig` was longer in the past and more files were being created then for separation, so this may have made more sense then, but now it is unnecessary
  - c.f. 7332077
@agilgur5 agilgur5 force-pushed the refactor-combine-tsconfig-handlers branch from f4535c1 to b7d1bd4 Compare August 25, 2022 05:19
@agilgur5 agilgur5 marked this pull request as ready for review August 25, 2022 05:19
@agilgur5 agilgur5 requested a review from ezolenko August 25, 2022 05:19
@ezolenko ezolenko merged commit 79053fe into ezolenko:master Aug 25, 2022
@agilgur5 agilgur5 deleted the refactor-combine-tsconfig-handlers branch July 2, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants