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.

Sign up for GitHub

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

Jump to bottom

Introducing classes to track issues #26

Merged
merged 7 commits into from
Oct 2, 2023
Merged

Introducing classes to track issues #26

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6e3bce9
introducing classes to track issues
ftomassetti Sep 27, 2023
d5f2a68
adding missing newline
ftomassetti Sep 27, 2023
6b2ee8d
making Result extend WithIssues
ftomassetti Sep 27, 2023
1ffec00
making WithIssues a dataclass
ftomassetti Sep 28, 2023
bb7c4c5
ignore .coverage report
ftomassetti Sep 28, 2023
293a63f
linting
ftomassetti Sep 28, 2023
8ddcf05
make WithIssues.issues not appear in constructor
ftomassetti Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions pylasu/parsing/results.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
from dataclasses import dataclass
from typing import List

from antlr4 import ParserRuleContext, Token

from pylasu.model import Source
from pylasu.validation.validation import WithIssues, IssueType, Issue


@dataclass
class FirstStageResult(WithIssues):
Copy link
Contributor

Choose a reason for hiding this comment

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

FirstStageResult should have an issues: List[Issue] = default_factory(list) property to avoid a dataclass missing field error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the issues list here would not be required if following the comment concerning WithIssues below

parse_tree: ParserRuleContext


@dataclass
class LexingResult(WithIssues):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as FirstStageResult and every other WithIssues subclass

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the issues list here would not be required if following the comment concerning WithIssues below

tokens: List[Token]


@dataclass
class IssuesErrorListener:
"""This Error Listener should be used with ANTLR lexers and parsers to capture issues"""
type: IssueType
source: Source
issues: WithIssues

def syntaxError(self, recognizer, offendingSymbol, line, column, msg, e):
self.issues.append(Issue(type=self.type, message=msg))

def reportAmbiguity(self, recognizer, dfa, startIndex, stopIndex, exact, ambigAlts, configs):
pass

def reportAttemptingFullContext(self, recognizer, dfa, startIndex, stopIndex, conflictingAlts, configs):
pass

def reportContextSensitivity(self, recognizer, dfa, startIndex, stopIndex, prediction, configs):
pass
5 changes: 5 additions & 0 deletions pylasu/validation/validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,8 @@ def semantic(message: str, severity: IssueSeverity = IssueSeverity.ERROR, positi
class Result:
root: Node
issues: List[Issue] = field(default_factory=list)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this extend WithIssues?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, I am just not sure of how the field definition works with inheritance, but I made the change

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid having to repeat the property for each subclass, we could change this to ... = [] rather than using the default_factory which is used for dataclasses. Defining this same class as dataclass would instead induce errors with the constructors.

Copy link
Member

@alessiostalla alessiostalla Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

issues = [] doesn't do what one would expect in Java or Kotlin – it's like a static field so all classes instances would potentially share the same (mutable) list.

I agree that field is only used with @dataclass so either WithIssues has to be decorated with @dataclass or it should be defined as follows:

class WithIssues:
    """Many classes have the necessity of tracking issues"""
    issues: List[Issue]

    def __init__(self):
        self.issues = []

Copy link
Contributor

@loradd loradd Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, then each subclass should be responsible of explicitly invoking the WithIssues constructor. If defining WithIssues as @dataclass, each field in the subclasses should be defined as optional and explicit names in creating new objects should be used to avoid confusion (issues would be the first constructor parameter).

EDIT: I think we should go for the solution proposed by @alessiostalla - i.e. adding init = False to the issues field in WithIssues



class WithIssues:
"""Many classes have the necessity of tracking issues"""
issues: List[Issue]