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

Allow 'id' as a variable name #593

Closed
mkordas opened this issue Jan 6, 2016 · 20 comments
Closed

Allow 'id' as a variable name #593

mkordas opened this issue Jan 6, 2016 · 20 comments
Assignees
Labels

Comments

@mkordas
Copy link
Contributor

mkordas commented Jan 6, 2016

Currently Qulice prohibits variable named just 'id', e.g.

final int id = readAttribute("id");

I don't see anything bad in using id as variable name, it is very common and widely accepted.

To overcome it, people use a lot of creativity. I've seen variables named like:

  • ide
  • ident
  • identifier
  • attrid
  • attribute
  • name
  • xid

and so on, instead of simple id.

I'd expect id to be allowed as final/non-final local variable names, method parameters, fields and constants (ID). It should be prohibited for catch parameters.

@krzyk
Copy link
Collaborator

krzyk commented Jan 7, 2016

@yegor256 please take a look at this, are you good with the change?

@yegor256
Copy link
Owner

yegor256 commented Jan 7, 2016

@krzyk make sense, I second this

@krzyk
Copy link
Collaborator

krzyk commented Jan 7, 2016

@davvd valid bug

@davvd
Copy link

davvd commented Jan 8, 2016

@davvd valid bug

@krzyk I added bug tag to this ticket

@davvd
Copy link

davvd commented Jan 11, 2016

@mkordas thanks for the ticket, your account was topped for 15 mins, payment 56936d48d7717a6a38000067

@krzyk
Copy link
Collaborator

krzyk commented Jan 12, 2016

@jrdalpra can you work on this?

@jrdalpra
Copy link
Contributor

@krzyk there's any test case to prove the issue?

@jrdalpra
Copy link
Contributor

@mkordas which tool is complaining about the attribute's name? Checkstyle? PMD?

@krzyk
Copy link
Collaborator

krzyk commented Jan 12, 2016

@jrdalpra you have to create a testcase to prove that it is no longer occurring after the changes

@krzyk
Copy link
Collaborator

krzyk commented Jan 12, 2016

@davvd assign @jrdalpra please

@mkordas
Copy link
Contributor Author

mkordas commented Jan 12, 2016

@jrdalpra it's Checkstyle complaining

@jrdalpra
Copy link
Contributor

@krzyk @mkordas the problem resides there https://github.com/teamed/qulice/blob/master/qulice-checkstyle/src/main/resources/com/qulice/checkstyle/checks.xml#L312-L315

Can I just pull the changes without a test case and create a puzzle for that test case?

Thanks.

@krzyk
Copy link
Collaborator

krzyk commented Jan 12, 2016

@jrdalpra without a test case we won't know if it reallyw works

@davvd
Copy link

davvd commented Jan 13, 2016

@davvd assign @jrdalpra please

@krzyk done. @jrdalpra the task is yours, please go ahead

@jrdalpra
Copy link
Contributor

@krzyk @davvd there's a PR #615 ready to review. thanks.

@jrdalpra
Copy link
Contributor

@mkordas can you close this issue? Thanks.

@mkordas
Copy link
Contributor Author

mkordas commented Jan 20, 2016

@jrdalpra you need to refer to pull request in at least one message

@jrdalpra
Copy link
Contributor

@mkordas Fixed at PR #621

@mkordas
Copy link
Contributor Author

mkordas commented Jan 20, 2016

@jrdalpra thanks!

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

davvd commented Jan 22, 2016

@jrdalpra Thanks a lot, I just topped your account for 30 mins, transaction ID AP-91W621665H2212107, total time was 193 hours and 17 mins; +30 added to your rating, at the moment it is: +278

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

5 participants