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

Audit compile time errors #280

Open
Blacksmoke16 opened this issue Apr 2, 2023 · 0 comments
Open

Audit compile time errors #280

Blacksmoke16 opened this issue Apr 2, 2023 · 0 comments
Labels
component:ecosystem Affects all components in the ecosystem kind:enhancement New functionality to an existing feature

Comments

@Blacksmoke16
Copy link
Member

With the introduction of crystal-lang/crystal#13260 and crystal-lang/crystal#13261, compile time error messages should now properly point to the related node. Up until now, Athena has been raising somewhat more verbose error messages than normal in order to help the user understand the error/where the problem is. For example:

# @[ADI::Register]
struct ShoutTransformer
  def transform(value : String) : String
    value.upcase
  end
end

@[ADI::Register(public: true)]
struct SomeAPIClient
  def initialize(@transformer : ShoutTransformer); end
end

ADI.container.some_api_client

If a service dependency is unable to be resolved, the error message would be like:

$ crystal run src/components/dependency_injection/test.cr 
Showing last frame. Use --error-trace for full trace.

There was a problem expanding macro 'finished'

Code in src/components/dependency_injection/src/service_container.cr:445:3

 445 | macro finished
       ^
Called macro defined in src/components/dependency_injection/src/service_container.cr:445:3

 445 | macro finished

Which expanded to:

 > 6 |     
 > 7 | 
 > 8 |     include ResolveArguments
           ^
Error: Failed to auto register service 'some_api_client'.  Could not resolve argument 'transformer : ShoutTransformer'.

Whereas after those PRs are merged, it'll be like:

$ ccrystal run src/components/dependency_injection/test.cr 
Using compiled compiler at /home/george/dev/git/crystal/.build/crystal
Showing last frame. Use --error-trace for full trace.

In src/components/dependency_injection/test.cr:12:18

 12 | def initialize(@transformer : ShoutTransformer); end
                     ^----------
Error: Failed to auto register service 'some_api_client'.  Could not resolve argument 'transformer : ShoutTransformer'.

Now that it actually points to the parameter that was unable to be resolved, it doesn't make sense to also include it in the error message.

I'm sure there are other cases where the message could be revised based on the better error traces. We should also make sure the compile time errors are pointing to the proper node as well. Otherwise it could make figuring out the cause even more confusing.

@Blacksmoke16 Blacksmoke16 added component:ecosystem Affects all components in the ecosystem kind:enhancement New functionality to an existing feature labels Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:ecosystem Affects all components in the ecosystem kind:enhancement New functionality to an existing feature
Development

No branches or pull requests

1 participant