Skip to content

Conversation

@wpoely86
Copy link
Member

@wpoely86 wpoely86 commented Feb 9, 2026

The code was not working

@wpoely86 wpoely86 requested a review from jrha February 9, 2026 11:31
@ned21
Copy link
Contributor

ned21 commented Feb 9, 2026

Can we have some more detail in the commit message please? I am particularly curious as to why this wasn't caught by the unit tests? Can we add one to check correct behaviour?

@wpoely86
Copy link
Member Author

wpoely86 commented Feb 9, 2026

Can we have some more detail in the commit message please? I am particularly curious as to why this wasn't caught by the unit tests? Can we add one to check correct behaviour?

Where should these tests run? 🙃

@wpoely86
Copy link
Member Author

wpoely86 commented Feb 9, 2026

Nevermind, found it.

There is no tests that uses that code. It directly tests with the subfunction.


# add ignored components
components_included.union(ignore_components)
components_included |= set(ignore_components)
Copy link
Member

Choose a reason for hiding this comment

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

For readability I'd prefer:

components_included.update(ignore_components)

or:

components_included = components_included.union(ignore_components)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a big fan of that. It's very verbose.

@jrha
Copy link
Member

jrha commented Feb 11, 2026

Whoops, we should add at least a test_good_component.pan in panc/src/main/scripts/panlint/test_files/.

@ned21
Copy link
Contributor

ned21 commented Feb 12, 2026

Code is fine (I think, I defer to James). But the documentation (aka commit logs) could be improved :)

panlint: fix import
Not clear to me what is being "fixed" here, or rather what was broken. Can you describe what problem you observed to which this is the fix?

Also I think it would be efficient to merge the other 2 commits into 1 with a nice explanation.

The results of the union operations needs to be stored again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants