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

Improve error message when "No valid constructors" #9501

Closed
wants to merge 1 commit into from

Conversation

fagossa
Copy link

@fagossa fagossa commented Jul 24, 2019

Pull Request Checklist

Fixes

Fixes #8474.

Purpose

This improves the error message when a Module that matches the constructor parameters specified could not be found.

Thanks a lot for your time!

@mkurz
Copy link
Member

mkurz commented Jul 24, 2019

A more descriptive commit message would be nice 😉

@fagossa
Copy link
Author

fagossa commented Jul 24, 2019

@mkurz I improved the commit message with a little more detail

@fagossa fagossa force-pushed the master branch 2 times, most recently from 5ba3a27 to df6d161 Compare July 25, 2019 07:42
@fagossa fagossa changed the title Fix 8474 [Fix 8474] Improve error message when "No valid constructors" Jul 26, 2019
@fagossa fagossa changed the title [Fix 8474] Improve error message when "No valid constructors" Fixes #8474 Improve error message when "No valid constructors" Jul 26, 2019
@octonato octonato changed the title Fixes #8474 Improve error message when "No valid constructors" Improve error message when "No valid constructors" Jul 30, 2019
Copy link
Contributor

@octonato octonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fagossa, the commit message is still "Fix #8474". You only changed the title of the PR.

Can you edit the message and do a push-forced? Tanks.

@fagossa
Copy link
Author

fagossa commented Jul 30, 2019

@renatocaval Actually I included more detail several lines bellow. I've ammended it so that the whole thing is visible in only one line:

Fix #8474. Improve error message when "No valid constructors"

Is that ok? Thanks for your time

@octonato
Copy link
Contributor

Yep, that's perfect.

Thanks @fagossa

…tors"

This commit does the following :
* List the available constructors while creating a module, when no available constructor is found
* Add a new unit test
* Add a new utility function in ConstructorUtils
case e =>
e.getMessage must contain("(com.typesafe.config.Config, java.lang.String, long)")
e.getMessage must contain("(com.typesafe.config.Config, java.lang.String)")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you got it the other way around.

If we throw such an exception to the users, we are just telling them which constructors we have found on the module they registered.

The actual suggestion was to inform the users that the module has the wrong constructor signature and therefore they need to adapt.

See @kubukoz comment here:
#8474 (comment)

And @gmethvin comment here:
#8474 (comment)

We should list the valid constructors, not those the user has defined.

Copy link
Contributor

@octonato octonato Aug 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick test and this is what I got:

class OnlyConfigModule(val config: Config) extends AbstractModule {
  override def configure(): Unit = ()
}

val env = Environment.simple()
val conf = Configuration(
  "play.modules.enabled" -> Seq(
    classOf[OnlyConfigModule].getName
  )
)

val located: Seq[Any] = Modules.locate(env, conf)

and it throws..

play.api.PlayException: No valid public constructors for play.api.inject.OnlyConfigModule. 
Expected one of: (com.typesafe.config.Config)

As a user, I would think: that's exactly the constructor that I defined, why it saying that it is expecting this constructor and still not using it.

@fagossa
Copy link
Author

fagossa commented Aug 5, 2019

:O You are right
I'll take a look at it

@cchantep
Copy link
Member

cchantep commented Mar 1, 2020

@fagossa any chance to finalize it?

@mkurz
Copy link
Member

mkurz commented Apr 14, 2020

This got fixed in #10150

@mkurz mkurz closed this Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"No valid constructors" if you only need Configuration
4 participants