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: expose error type #276

Merged
merged 7 commits into from
Mar 18, 2024

Conversation

multivoltage
Copy link
Contributor

I tried to export EtaError types introducing new types like @nebrelbug did in THIS issue.

Some point about he PR:

  • I put an EtaXXXError instead EtaError in all point. So at the and I asked myself "Is now useless EtaError type? Can I remove it"
  • I added a test for each error interface. I tried to throw starting from top-level api. For example using .render() even if the exception is inside .readFile()
  • I added never as return type since for me a function that throw 100% of times an error equals to a function that never run an error (token from TS docs)

I don't know if this PR follow exactly the structure described here:
#243 (comment)

But probably this PR can be a starting point and I'm happy to change some part after review

@nebrelbug
Copy link
Collaborator

@multivoltage thanks for the PR! This looks great. Just a few changes before I can merge:

  • Rename EtaRuntimeErr to EtaRuntimeError
  • I think it would be great if all Eta errors extended a base class EtaError, which could just be declared like class EtaError extends Error {}.

I appreciate the TypeScript updates!

@nebrelbug
Copy link
Collaborator

Also if you can make sure to follow the Commitlint format for commit messages, that's what's causing tests to fail right now. Thanks!!

@multivoltage
Copy link
Contributor Author

npx commitlint --from ee0a1d44addd162b53b20329799b93628fcba22e~6 --to ee0a1d44addd162b53b20329799b93628fcba22e --verbose
⧗   input: feat: export EtaNameResolutionError
✔   found 0 problems, 0 warnings
⧗   input: feat: add use case for EtaFileResolutionError
✔   found 0 problems, 0 warnings
⧗   input: feat: export EtaFileResolutionError
✔   found 0 problems, 0 warnings
⧗   input: feat: use custom error for compile() api
✔   found 0 problems, 0 warnings
⧗   input: feat: export EtaRuntimeError
✔   found 0 problems, 0 warnings
⧗   input: feat: export EtaParseError
✔   found 0 problems, 0 warnings

In my local pc seems ok (node 18)

@nebrelbug
Copy link
Collaborator

@multivoltage hmm, you're right. I'm not actually sure what the problem is, but it seems like it may be a problem with the version of commitlint that the GitHub Action is installing.

@multivoltage
Copy link
Contributor Author

@nebrelbug I replicated issue.
In. my local pc like I said I run "npx commitlint....".

But in you yaml you install before

      - name: Install commitlint
        run: |
          npm install conventional-changelog-conventionalcommits
          npm install commitlint@latest

and then use npx.

I think you have 2 possible fix:

  1. remove step to install the two deps and keep going to use "npx commitlint" like in my local pc.
  2. Installing same deps in my pc and run "npx commitlint" the error is the same. I solved running also @commitlint/format

Please give me a feedback. Do you plan to fix this in seperate pr?

@nebrelbug
Copy link
Collaborator

@multivoltage I'll fix the commitlint issue in a separate PR later. In the meantime, I'll merge anyways if you can fix the issues I mentioned above.

@multivoltage
Copy link
Contributor Author

@nebrelbug
Sorry. I did not notice your first comment :)

I think it would be great if all Eta errors extended a base class EtaError, which could just be declared like class EtaError extends Error {}.

I thought the same thing inirtially but then I ask my self?

What EtaError give me more than a basic Erro? Anyway I will do it :)

@multivoltage multivoltage force-pushed the feature/expose-error-type branch from ee0a1d4 to e4e8ebb Compare March 17, 2024 18:20
@nebrelbug
Copy link
Collaborator

@multivoltage thank you! Having a base EtaError class will be useful if people want to catch all errors that Eta throws.

@nebrelbug nebrelbug changed the title Feature/expose error type feat: expose error type Mar 18, 2024
@nebrelbug nebrelbug merged commit 70bd7e3 into eta-dev:main Mar 18, 2024
3 checks passed
@nebrelbug
Copy link
Collaborator

@multivoltage released in Eta 3.4.0:)

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