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

Ignored attributes added in robula model #196

Merged

Conversation

Bojana33
Copy link

@Bojana33 Bojana33 commented Aug 2, 2024

No description provided.

@Bojana33 Bojana33 requested a review from ivnglkv August 2, 2024 08:31
utils/robula.py Outdated
Comment on lines 161 to 163
if config["ignored_attributes"] is not None and len(config["ignored_attributes"]) != 0:
self.attribute_black_list.extend(config["ignored_attributes"])
self.attribute_black_list = list(dict.fromkeys(self.attribute_black_list))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be just

Suggested change
if config["ignored_attributes"] is not None and len(config["ignored_attributes"]) != 0:
self.attribute_black_list.extend(config["ignored_attributes"])
self.attribute_black_list = list(dict.fromkeys(self.attribute_black_list))
if config.ignored_attributes:
self.attribute_black_list.extend(config.ignored_attributes)

Pydantic guarantees us that ignored_attributes is either None, either list and bool([]) is False so it doesn't make sense to check the length here.

And this line changes nothing

self.attribute_black_list = list(dict.fromkeys(self.attribute_black_list))

Copy link
Author

Choose a reason for hiding this comment

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

self.attribute_black_list = list(dict.fromkeys(self.attribute_black_list))
With this line I wanted to make sure that there are no duplicates when we extend the list.

Copy link
Contributor

Choose a reason for hiding this comment

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

In such cases it's better to leave comment explaining this, because as you can see, it was really not obvious :)
Can we convert self.attribute_black_list to set to avoid having duplicates there?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we can do it with the set too. But in this way we will keep the order of the elements. I will add a comment.

Copy link
Contributor

@ivnglkv ivnglkv Aug 2, 2024

Choose a reason for hiding this comment

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

But in this way we will keep the order of the elements

Does it matter?

Copy link
Author

Choose a reason for hiding this comment

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

Well in this case it actually doesn't :)

@Bojana33 Bojana33 requested a review from ivnglkv August 2, 2024 12:00
utils/robula.py Outdated
Comment on lines 161 to 164
if config.ignored_attributes:
self.attribute_black_list.extend(config.ignored_attributes)
# Remove duplicates from the list
self.attribute_black_list = list(set(self.attribute_black_list))
Copy link
Contributor

@ivnglkv ivnglkv Aug 4, 2024

Choose a reason for hiding this comment

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

What I actually meant was

class RobulaPlus:
    def __init__(self, element, document, config):
        ...
        self.attribute_black_list = {
            "href",
            "jdn-hash",
             ...,
        }
        ...
        if config.ignored_attributes:
            self.attribute_black_list.update(config.ignored_attributes)

It's much cleaner code and we do less work here. We don't even have to mention something about duplicates in this case.

utils/robula.py Outdated
@@ -158,6 +158,9 @@ def __init__(self, element, document, config):
self.element = element
self.document = document

if config["ignored_attributes"] is not None and len(config["ignored_attributes"]) != 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just

Suggested change
if config["ignored_attributes"] is not None and len(config["ignored_attributes"]) != 0:
if config["ignored_attributes"]:

?

@Bojana33 Bojana33 merged commit 5193ba5 into develop Aug 5, 2024
2 checks passed
@Bojana33 Bojana33 deleted the issue_191-ignore-attributes-for-locators-generation branch August 5, 2024 11:48
@Bojana33 Bojana33 restored the issue_191-ignore-attributes-for-locators-generation branch August 5, 2024 11:48
@ivnglkv ivnglkv deleted the issue_191-ignore-attributes-for-locators-generation branch August 5, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants