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

Parse 61131-3 Classes #225

Merged
merged 32 commits into from
Aug 12, 2021
Merged

Parse 61131-3 Classes #225

merged 32 commits into from
Aug 12, 2021

Conversation

ulmer-a
Copy link
Collaborator

@ulmer-a ulmer-a commented Aug 5, 2021

After this PR, rusty will be able to parse classes. Closes #219. Please merge PR #211 first.

Note that I accidently rebased the changes from PR #211 into this branch (first 10 commits below). Would have been better to merge it... It's going to be squashed anyway, though.

Deviation from the standard: We do not support VAR_EXTERNAL constructs since they are rarely used. Namespaces, Inheriting and interface have not been implemented, yet.

@ulmer-a ulmer-a self-assigned this Aug 5, 2021
@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #225 (7e44d5a) into master (7e18300) will increase coverage by 0.06%.
The diff coverage is 98.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage   93.52%   93.58%   +0.06%     
==========================================
  Files          34       34              
  Lines        9054     9173     +119     
==========================================
+ Hits         8468     8585     +117     
- Misses        586      588       +2     
Impacted Files Coverage Δ
src/index.rs 94.40% <ø> (ø)
src/index/visitor.rs 96.18% <0.00%> (-0.37%) ⬇️
src/lexer.rs 95.32% <ø> (ø)
src/parser.rs 97.86% <99.42%> (+0.27%) ⬆️
src/ast.rs 87.83% <100.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e18300...7e44d5a. Read the comment docs.

@ulmer-a ulmer-a marked this pull request as ready for review August 6, 2021 08:14
@ulmer-a ulmer-a marked this pull request as draft August 6, 2021 08:15
@ulmer-a ulmer-a changed the title Parse 61131-3 Classes (WIP) Parse 61131-3 Classes Aug 6, 2021
@ulmer-a ulmer-a marked this pull request as ready for review August 6, 2021 12:40
@ulmer-a ulmer-a requested a review from riederm August 6, 2021 12:40
Copy link
Collaborator

@ghaith ghaith left a comment

Choose a reason for hiding this comment

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

Just some discussion points

src/ast.rs Outdated Show resolved Hide resolved
src/parser.rs Show resolved Hide resolved
src/ast.rs Outdated Show resolved Hide resolved
src/ast.rs Show resolved Hide resolved
@ulmer-a ulmer-a requested review from ghaith and removed request for riederm August 10, 2021 09:48
Copy link
Collaborator

@ghaith ghaith left a comment

Choose a reason for hiding this comment

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

I think we just need a test for the FB case. Otherwise this looks good. I'll test it a bit locally though before I give the last approval

src/parser/tests/class_parser_tests.rs Outdated Show resolved Hide resolved
src/parser/tests/class_parser_tests.rs Show resolved Hide resolved
@ghaith
Copy link
Collaborator

ghaith commented Aug 10, 2021

What I think might also be good are index tests where we verify instances of a class are initialized correctly, and that we can find methods on classes.

Copy link
Collaborator Author

@ulmer-a ulmer-a left a comment

Choose a reason for hiding this comment

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

Not entirely sure if the following tests are correct, but I think so

src/index/tests/index_tests.rs Show resolved Hide resolved
src/index/tests/index_tests.rs Show resolved Hide resolved
@ulmer-a ulmer-a requested a review from ghaith August 11, 2021 12:41
src/index/tests/index_tests.rs Show resolved Hide resolved
src/index/tests/index_tests.rs Show resolved Hide resolved
@ulmer-a ulmer-a merged commit a1c7ea6 into PLC-lang:master Aug 12, 2021
@ulmer-a ulmer-a deleted the classes branch August 12, 2021 09:25
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.

Support Classes - part1: parse classes
2 participants