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.

Sign up for GitHub

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

Jump to bottom

Ignored attributes added in robula model #196

Merged
merged 6 commits into from
Aug 5, 2024
Merged

Ignored attributes added in robula model #196

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dc1bab5
Ignored attributes added in robula model
Aug 2, 2024
cbda145
Lint fix
Aug 2, 2024
a299696
Small fix
Aug 2, 2024
2271b4e
Convert attr black_list in set
Aug 5, 2024
26bcd45
Condition changed
Aug 5, 2024
683a140
Condition fix
Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class RobulaSettingsModel(BaseModel):
allow_indexes_in_the_middle: bool = True
allow_indexes_at_the_end: bool = True
advanced_calculation: bool = False
ignored_attributes: Optional[List[str]] = None


class XPathGenerationModel(TaskIdModel):
Expand Down
4 changes: 4 additions & 0 deletions utils/robula.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,10 @@ 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"]:

?

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 •
edited
Loading

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 :)


def check_for_time_limit(self, start_time):
evaluation_time_in_seconds = (
datetime.datetime.now() - start_time
Expand Down