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

Qulice allows catch parameters not following naming convention #627

Closed
mkordas opened this issue Jan 19, 2016 · 23 comments
Closed

Qulice allows catch parameters not following naming convention #627

mkordas opened this issue Jan 19, 2016 · 23 comments
Assignees
Labels

Comments

@mkordas
Copy link
Contributor

mkordas commented Jan 19, 2016

Currently Qulice does not complain on the following catch parameter names:

try {
    ...
} catch(IllegalStateException ex_invalid_1) {
    ...
}
try {
    ...
} catch(IllegalStateException $xxx) {
    ...
}

I'd expect violation on both of these names. Only ex or 3-12 lowercase latin characters should be allowed.

Pattern should be ^(ex|[a-z]{3,12})$. Moreover, rest of patterns should be unified to follow the convention: replace ^id$|^[a-z]{3,12}$ with ^(id|[a-z]{3,12})$ in LocalFinalVariableName, LocalVariableName, MemberName, ParameterName.

@krzyk
Copy link
Collaborator

krzyk commented Jan 19, 2016

@mkordas doesn't it work already?

        <module name="CatchParameterName">
            <property name="format" value="^ex|[a-z]{3,12}$"/>
        </module>

Is this configuration wrong?

@mkordas
Copy link
Contributor Author

mkordas commented Jan 20, 2016

@krzyk both names are matching our regex: http://regexr.com/3cki7

@krzyk
Copy link
Collaborator

krzyk commented Jan 21, 2016

@mkordas so the () are missing, strange, those shouldn't be required, but they are.
Could you also add to the description that we should follow the convention of ^(ex|[a-z]{3,12})$ and replace all^id$|^[a-z]{3,12}$ with ^(id|[a-z]{3,12})$ (so the list includes LocalFinalVariableName, LocalVariableName, MemberName, ParameterName)?

@krzyk
Copy link
Collaborator

krzyk commented Jan 22, 2016

@mkordas ping

@mkordas
Copy link
Contributor Author

mkordas commented Jan 22, 2016

@krzyk done, thanks for pinging

@krzyk
Copy link
Collaborator

krzyk commented Jan 22, 2016

@davvd valild bug

@krzyk
Copy link
Collaborator

krzyk commented Jan 22, 2016

@davvd valid bug

@krzyk
Copy link
Collaborator

krzyk commented Jan 22, 2016

@davvd this is postponed

@davvd
Copy link

davvd commented Jan 22, 2016

@davvd valild bug

@krzyk tagged this issue with "bug"

@davvd davvd added the bug label Jan 22, 2016
@davvd
Copy link

davvd commented Jan 22, 2016

@davvd valid bug

@krzyk added "bug" tag to this issue

@davvd
Copy link

davvd commented Jan 22, 2016

@davvd this is postponed

@krzyk right, I added "postponed" label

@davvd
Copy link

davvd commented Jan 22, 2016

@davvd this is postponed

@krzyk got it, someone else will be assigned soon

@davvd
Copy link

davvd commented Jan 25, 2016

@mkordas thanks for the report, I topped your acc for 15 mins, payment ID 56a5fb478cc94d3272000322

@davvd davvd added DEV labels Jan 25, 2016
@krzyk
Copy link
Collaborator

krzyk commented Jan 26, 2016

@davvd this is not postponed

@krzyk
Copy link
Collaborator

krzyk commented Jan 26, 2016

@davvd assign @kitsook

@davvd
Copy link

davvd commented Jan 26, 2016

@davvd this is not postponed

@krzyk sure, thanks, I removed "postponed" tag to it

@davvd davvd removed the postponed label Jan 26, 2016
@davvd
Copy link

davvd commented Jan 26, 2016

@davvd assign @kitsook

@krzyk done. @kitsook you are assigned to this task. please proceed...

@kitsook
Copy link
Contributor

kitsook commented Jan 26, 2016

@krzyk the regular expressions of LocalFinalVariableName, LocalVariableName, MemberName, and ParameterName are working fine and do not require changes

^id$|^[a-z]{3,12}$ works because it means there are two alternatives:

  • between line start and end, contain "id"
  • between line start and end, contain 3 to 12 lowercase alphabets

^ex|[a-z]{3,12}$ of CatchParameterName doesn't work as expected as it means there are two alternatives:

  • line start with "ex"
  • line end with 3 to 12 lowercase alphabets

^ex|[a-z]{3,12}$ can be fixed with ^ex$|^[a-z]{3,12}$ or the proposed ^(ex|[a-z]{3,12})$

@krzyk
Copy link
Collaborator

krzyk commented Jan 26, 2016

@kitsook let's make all the regexes you mentioned to the ^(...)$ convention, it is more readable, also we need a test for the CatchParameterName so we will be sure there won't be any regression

kitsook added a commit to kitsook/qulice that referenced this issue Jan 26, 2016
@kitsook
Copy link
Contributor

kitsook commented Jan 26, 2016

@Dvvd PR #651 created

kitsook added a commit to kitsook/qulice that referenced this issue Jan 27, 2016
@kitsook
Copy link
Contributor

kitsook commented Jan 28, 2016

@mkordas PR #651 merged. Please close the issue

@mkordas
Copy link
Contributor Author

mkordas commented Jan 28, 2016

@kitsook thanks!

@mkordas mkordas closed this as completed Jan 28, 2016
@davvd
Copy link

davvd commented Feb 1, 2016

@kitsook 38 mins was added to your account, many thanks for your contribution (payment AP-5BA49698PH173703C)! The task took 70 hours and 22 mins.

m=4222, that's why a bonus here for fast delivery

+38 added to your rating, current score is: +98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants