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

Rule Proposal: warn if # is used in by.id locators #67

Closed
alecxe opened this issue Jan 18, 2017 · 6 comments · Fixed by #68
Closed

Rule Proposal: warn if # is used in by.id locators #67

alecxe opened this issue Jan 18, 2017 · 6 comments · Fixed by #68

Comments

@alecxe
Copy link
Owner

alecxe commented Jan 18, 2017

Example of a violation:

element(by.id('#1484755825715-7-uiGrid-0007-cell'));

Correct usage:

element(by.id('1484755825715-7-uiGrid-0007-cell'))
@eLRuLL
Copy link
Contributor

eLRuLL commented Jan 19, 2017

let me take this, do you have a recommendation for the rule name?

@alecxe
Copy link
Owner Author

alecxe commented Jan 19, 2017

@eLRuLL sure!

One idea is to have a general valid-by-id and check not only for # but for overall proper id attribute value (reference).

Another, is to keep this rule simple - no-hash-in-by-id?

What do you think? Thanks.

@eLRuLL
Copy link
Contributor

eLRuLL commented Jan 19, 2017

I honestly think we should approach this as valid id (so valid_by_id) with a regex to match html4 rule:

ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by any number of letters, digits ([0-9]), hyphens ("-"), underscores ("_"), colons (":"), and periods (".").

@robinj
Copy link

robinj commented Jan 19, 2017

@eLRuLL ignore my previous post. I think you are right. It makes more sense to have that rather than potentially adding many rules to cover all invalid cases.

@alecxe
Copy link
Owner Author

alecxe commented Jan 19, 2017

@robinj @eLRuLL sounds great, let's cover the general id validness. Thanks.

@alecxe
Copy link
Owner Author

alecxe commented Jan 20, 2017

@eLRuLL thanks for the pull request!

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

Successfully merging a pull request may close this issue.

3 participants