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: consolidate diagnostics funcs into single file #416

Merged

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Aug 30, 2022

Summary

  • Move diagnostics funcs into a single file named diagnostics.ts
  • Add a unit test for convertDiagnostic
  • Tiny bugfix \n -> formatHost.getNewLine()
    • This doesn't seem to have ever affected anyone; I'm guessing either Node or people's terminals auto-translated the newlines b/t OSes or something? So this is more of a semantic bugfix in that sense

Details

refactor:

  • move IDiagnostic and convertDiagnostic from tscache to print-diagnostics, then rename it to diagnostics.ts

    • the diagnostic funcs being in tscache always felt like a strange place for them
      • especially when parse-tsconfig or print-diagnostics would import them from tscache, which sounded like an unrelated file
  • may want to move diagnostics-format-host into this file as well, as it is only used in this file

    • leaving as is for now, limiting this change to a smaller one

test: add unit test for convertDiagnostic

  • this was previously only covered in integration tests
    • since unit tests don't currently touch index or tscache, and convertDiagnostic was previously in tscache
      • another reason why consolidating these functions into one file made sense

fix(diagnostics): use formatHost.getNewLine() instead of \n

  • since new lines are host OS specific

- move `IDiagnostic` and `convertDiagnostic` from `tscache` to `print-diagnostics`, then rename it to `diagnostics.ts`
  - the diagnostic funcs being in `tscache` always felt like a strange place for them
    - especially when `parse-tsconfig` or `print-diagnostics` would import them from `tscache`, which sounded like an unrelated file

- may want to move `diagnostics-format-host` into this file as well, as it is _only_ used in this file
  - leaving as is for now, limiting this change to a smaller one
- this was previously only covered in integration tests
  - since unit tests don't currently touch `index` or `tscache`, and `convertDiagnostic` was previously in `tscache`
    - another reason why consolidating these functions into one file made sense
@agilgur5 agilgur5 added kind: internal Changes only affect the internals, and _not_ the public API or external-facing docs scope: tests Tests could be improved. Or changes that only affect tests labels Aug 30, 2022
@agilgur5 agilgur5 force-pushed the refactor-consolidate-diagnostics-file branch from 7072019 to 2d3cfe3 Compare August 30, 2022 21:08
@agilgur5 agilgur5 marked this pull request as draft August 30, 2022 21:09
- since new lines are host OS specific

- this fixes the `convertDiagnostic` test on Windows too, where it was failing
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Aug 30, 2022

  • Tiny bugfix \n -> formatHost.getNewLine()
    • This doesn't seem to have ever affected anyone; I'm guessing either Node or people's terminals auto-translated the newlines b/t OSes or something? So this is more of a semantic bugfix in that sense

Well this was pretty ironic -- the unit test I added actually fails on Windows without this fix (and a test adjustment). Given that I'm on Mac, I literally didn't see this until it hit CI after I wrote up the PR. Turns out the test confirms this behavior!

@agilgur5 agilgur5 marked this pull request as ready for review August 30, 2022 21:21
@agilgur5
Copy link
Collaborator Author

@ezolenko friendly ping in case you just hadn't seen this PR (since you merged some others, but not this one)

I also think we should be good to release 0.34.0, mainly containing #406 as that's a big enough change as is.
0.33.0 has already been out a few weeks without issues, so don't think we need to worry about releasing two minors shortly after each other anymore.

There's a couple more things that I'm working on, but those will probably take some time (not spending as much time on rpt2 anymore since I've fixed the vast majority of issues! 🙂), so don't want to hold back releasing #406 because of that, since it's a big fix for what is probably the single most common issue in this repo.
Also the stuff I'm working on is mostly bugfixes and optimizations for cache/watch mode (i.e. follow-ups to #386), which can go out whenever as patch releases. #228 and #86 are the only real leftover bugs, and fixes to those probably should be in a separate minor than #406 anyway, IMO. Those might also be a few weeks away too (or more or less -- don't have much of a timeline on those in general), as they're not high priority in my "OSS todo list".

@ezolenko ezolenko merged commit ba26293 into ezolenko:master Sep 12, 2022
@ezolenko
Copy link
Owner

ezolenko commented Sep 12, 2022

@agilgur5 0.34.0 is out on npm now, pls check if release notes on github make sense. Great work, thanks!

@agilgur5 agilgur5 deleted the refactor-consolidate-diagnostics-file 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 scope: tests Tests could be improved. Or changes that only affect tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants