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

Fix bad-import-yang-version handling #237

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

esmasth
Copy link
Contributor

@esmasth esmasth commented Mar 16, 2024

It is allowed to import YANG 1.1 modules from YANG 1.0 when importing without revision-date. bad-import-yang-version was flagged even when a YANG 1.1 module was imported by YANG 1.0 module without specifying revision-date and is now fixed.

This fix addresses #228

It is allowed to import YANG 1.1 modules from YANG 1.0 when importing
without revision-date. bad-import-yang-version was flagged even when
a YANG 1.1 module was imported by YANG 1.0 module without specifying
revision-date and is now fixed.

This fix addresses TypeFox#228

Signed-off-by: Siddharth Sharma <[email protected]>
@esmasth esmasth marked this pull request as ready for review March 16, 2024 10:32
Added build settings for vscode

Signed-off-by: Siddharth Sharma <[email protected]>
@dhuebner
Copy link
Member

@esmasth
Thanks a lot for your contribution!
I will re-view and merge your PR this week.

Copy link
Member

@dhuebner dhuebner left a comment

Choose a reason for hiding this comment

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

I added a minor improvement suggestion, otherwise it looks good to me.

if(baseModuleVersion != importedModuleVersion) {
val message = '''Cannot import a version «importedModuleVersion» module in a version «baseModuleVersion» module.''';
val revisionDate = importStatement.substatementsOfType(RevisionDate)
if (!revisionDate.nullOrEmpty && baseModuleVersion != importedModuleVersion) {
Copy link
Member

@dhuebner dhuebner Mar 20, 2024

Choose a reason for hiding this comment

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

[nit] substatementsOfType can not return null, you can just use .empty.
[nit] revisionDate is a lazy evaluated iterable, if you move it after baseModuleVersion != importedModuleVersion it will only be calculated in the erroring case and save some computation time.

Proposed change:

val revisionDate = importStatement.substatementsOfType(RevisionDate)
if (baseModuleVersion != importedModuleVersion && !revisionDate.empty) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing this in following update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@dhuebner
Copy link
Member

@esmasth
Btw. you can use addIssue(...) instead of error(...) to make the validation severity configurable. The severity will then be fetched from the settings, or the default defined in io.typefox.yang.validation.IssueCodes will be used.

@esmasth
Copy link
Contributor Author

esmasth commented Mar 21, 2024

@dhuebner Thanks, is it decided then to make these issues configurable? I rather agree with your statement on yang-vscode that it's a potential to break the engine if we allow unambiguously wrong input. warning severity configurability would make sense to me for this code, if we didn't actually fix the bug.

@dhuebner
Copy link
Member

@esmasth

is it decided then to make these issues configurable?

No, but we can allow to configure it by the user. I think the user can then decide what to do.

if we didn't actually fix the bug.
Your PR fixes the validation, or not?

Do you what to apply the suggestions I made, or should I merge your PR as it is?

@esmasth
Copy link
Contributor Author

esmasth commented Mar 22, 2024

I will do the late evaluation and .empty change as per your good feedback. Just didn't get around to it.

It might be good to have some strictness for MUST/SHALL statement violations in YANG RFC with some consistent policy for the project. Maybe good to discuss that on TypeFox/yang-vscode#40 to conclude?

@dhuebner
Copy link
Member

@esmasth
Good idea. Let's do it. I'm not using Yang as a developing language, so professional input and discussions a very welcome :)

substatementsOfType does not return null and hence only empty needs
to be checked. Also changed the order of checks to improve performance.

Signed-off-by: Siddharth Sharma <[email protected]>
@esmasth
Copy link
Contributor Author

esmasth commented Mar 22, 2024

Ok with merging this now, if you are.

@dhuebner dhuebner merged commit 0e0d1c6 into TypeFox:master Mar 22, 2024
1 check passed
@dhuebner
Copy link
Member

@esmasth
Merged. Thanks for contributing again :)

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