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

Validator cleanups #961

Merged
merged 4 commits into from
Feb 13, 2022
Merged

Validator cleanups #961

merged 4 commits into from
Feb 13, 2022

Conversation

edwardalee
Copy link
Collaborator

This PR cleans up LFValidator:

  • Organize methods for easier browsing.
  • Make fields private that are not used anywhere else.
  • Remove fields that are not used anywhere at all.
  • Rename private static constants to use consistent convention.

This should be merged quickly because of the major file structure reorganization, which will likely lead to conflicts.

There is only one minor functional change: The checkSTPOffset() method was not actually performing any check. Now it is (so there is a supporting change in ModelInfo).

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

I don't see any red flags here, but the diff is impossible to read because many things were moved around. I would be great if you could also add a unit test for the newly created check.

Copy link
Contributor

@Soroosh129 Soroosh129 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Thank you for adding the check for the STP offset.

Would this also solve #903?

Edit: I agree with @lhstrh that adding tests for the newly added checks would really help.

org.lflang/src/org/lflang/ModelInfo.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/validation/LFValidator.java Outdated Show resolved Hide resolved
edwardalee and others added 2 commits February 13, 2022 11:26
Co-authored-by: Soroush Bateni <soroosh129@gmail.com>
Co-authored-by: Soroush Bateni <soroosh129@gmail.com>
@edwardalee edwardalee merged commit a061ea6 into master Feb 13, 2022
@edwardalee
Copy link
Collaborator Author

This does not fix #903. In fact, the check of STP still does not work (it didn't work before). Neither does the check for overflowing deadlines, which uses the same architecture. It needs to be a separate PR to fix those checks. This PR does not change any functionality at all.

@edwardalee edwardalee deleted the validator-cleanups branch February 13, 2022 19:28
@lhstrh lhstrh added the refactoring Code quality enhancement label Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code quality enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants