Skip to content

Fix duplicated and missing declarations#64

Merged
droideck merged 1 commit intopyasn1:mainfrom
Avasam:duplicated-and-missing-declarations
Aug 10, 2024
Merged

Fix duplicated and missing declarations#64
droideck merged 1 commit intopyasn1:mainfrom
Avasam:duplicated-and-missing-declarations

Conversation

@Avasam
Copy link
Copy Markdown

@Avasam Avasam commented Apr 1, 2024

Various duplicated declarations and a missing import I found

Comment thread tests/type/test_univ.py

class Sequence(univ.Sequence):
componentType = namedtype.NamedTypes(
namedtype.OptionalNamedType('name', univ.OctetString())
Copy link
Copy Markdown
Author

@Avasam Avasam Apr 1, 2024

Choose a reason for hiding this comment

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

The current definition passes an empty string to OctetString:
namedtype.OptionalNamedType('name', univ.OctetString(''))

@Avasam Avasam force-pushed the duplicated-and-missing-declarations branch from 33a3384 to 0f78ee9 Compare April 1, 2024 04:33
@Avasam Avasam force-pushed the duplicated-and-missing-declarations branch from 1aa9ce0 to 445dd82 Compare July 30, 2024 18:00

from tests.base import BaseTestCase

from pyasn1.error import PyAsn1Error
Copy link
Copy Markdown
Author

@Avasam Avasam Aug 1, 2024

Choose a reason for hiding this comment

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

This missing import implies that the except PyAsn1Error path in these tests was never being taken (or the tests not run). That's something you might wanna look into. Maybe the try-except should be removed instead ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We haven't had any failures in the tests. That is good:)
I think leaving the excepts in the tests is okay. If the test fails, it'll help explain what was expected.

But yes, we need the import fix you did.

Copy link
Copy Markdown

@droideck droideck left a comment

Choose a reason for hiding this comment

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

LGTM!


from tests.base import BaseTestCase

from pyasn1.error import PyAsn1Error
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We haven't had any failures in the tests. That is good:)
I think leaving the excepts in the tests is okay. If the test fails, it'll help explain what was expected.

But yes, we need the import fix you did.

@droideck droideck merged commit d1381d4 into pyasn1:main Aug 10, 2024
@Avasam Avasam deleted the duplicated-and-missing-declarations branch August 10, 2024 03:00
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