Exception message not informative enough for users on incorrect parent #2186

Closed
mkordas opened this Issue Sep 15, 2015 · 9 comments

Comments

Projects
None yet
6 participants
@mkordas
Contributor

mkordas commented Sep 15, 2015

When running Checkstyle with the following config:

    <module name="Checker">
        <module name="TreeWalker">
            <module name="RegexpMultiline"/>
        </module>
    </module>

I get not very informative message:

cannot initialize module TreeWalker - TreeWalker is not allowed as a parent of RegexpMultiline

I'd expect information that RegexpMultiline has parent TreeWalker, but should have Checker as parent.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 15, 2015

Member

this issue might be related #898 .

Member

romani commented Sep 15, 2015

this issue might be related #898 .

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 6, 2017

Member

@romani Should new message be:
cannot initialize module TreeWalker - TreeWalker is not allowed as a parent for RegexpMultiline and must have Checker as a parent
If so, what should message be if parent is unknown because user used some custom class we don't recognize? Should it just default to the current message?

Member

rnveach commented Jan 6, 2017

@romani Should new message be:
cannot initialize module TreeWalker - TreeWalker is not allowed as a parent for RegexpMultiline and must have Checker as a parent
If so, what should message be if parent is unknown because user used some custom class we don't recognize? Should it just default to the current message?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 6, 2017

Member

Message is ok.

Yes, if unknown parent, lets use default message

Member

romani commented Jan 6, 2017

Message is ok.

Yes, if unknown parent, lets use default message

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 6, 2017

Member

Message cannot be fully changed in AutomaticBean because to identify parent we need to know the child's type and to know the type we need an instance to work with. We have no instance there currently.
If we want to continue with this change there, AutomaticBean.setupChild would have to be changed to try to instantiate the child first. If we are going this far, maybe we should instantiate the child before we even call setupChild and pass the instance along as all beans need to do this (TreeWalker and Checker) . To do this would require a new issue as it is an API change and would require CS 8.

Checker and TreeWalker messages can still be changed now.

Member

rnveach commented Jan 6, 2017

Message cannot be fully changed in AutomaticBean because to identify parent we need to know the child's type and to know the type we need an instance to work with. We have no instance there currently.
If we want to continue with this change there, AutomaticBean.setupChild would have to be changed to try to instantiate the child first. If we are going this far, maybe we should instantiate the child before we even call setupChild and pass the instance along as all beans need to do this (TreeWalker and Checker) . To do this would require a new issue as it is an API change and would require CS 8.

Checker and TreeWalker messages can still be changed now.

@rnveach rnveach self-assigned this Jan 6, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 6, 2017

Member

@rnveach ,

no, we do not need complication for such simple issue.
Is we can not goo message for user, we should stay with what we have and extend general wording:

cannot initialize module TreeWalker - TreeWalker is not allowed as a parent of RegexpMultiline. Please review 'Parent Module' section for this Check in web documentation if Checks is standard.

Member

romani commented Jan 6, 2017

@rnveach ,

no, we do not need complication for such simple issue.
Is we can not goo message for user, we should stay with what we have and extend general wording:

cannot initialize module TreeWalker - TreeWalker is not allowed as a parent of RegexpMultiline. Please review 'Parent Module' section for this Check in web documentation if Checks is standard.

@rnveach rnveach removed their assignment Mar 2, 2017

@rnveach rnveach added the easy label Mar 2, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@nikshinde1996

This comment has been minimized.

Show comment
Hide comment

I am on it

@subkrish

This comment has been minimized.

Show comment
Hide comment
@subkrish

subkrish Apr 6, 2017

Contributor

@rnveach Since @nikshinde1996 did not continue to work on this, I would like to take this issue over. I am on it.

Contributor

subkrish commented Apr 6, 2017

@rnveach Since @nikshinde1996 did not continue to work on this, I would like to take this issue over. I am on it.

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Apr 6, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Apr 6, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Apr 6, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Apr 6, 2017

subkrish pushed a commit to subkrish/checkstyle that referenced this issue Apr 9, 2017

romani added a commit that referenced this issue Apr 11, 2017

@romani romani added the bug label Apr 11, 2017

@romani romani added this to the 7.7 milestone Apr 11, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 11, 2017

Member

Fix is merged

Member

romani commented Apr 11, 2017

Fix is merged

@romani romani closed this Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment