Add external token classification - #18
Conversation
|
At the moment the functest recipe assumes that re-classify is installed locally. That is wrong! |
|
This PR needs an accompanying change in the specification to support external re-classifiers. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to invoke an external program to alter the initial classification of identifiers, operators and brackets, selectively overriding the dynamic discovery of token roles. This provides additional flexibility for Monogram to cover more use-cases with more precision through carefully constrained grammars.
Key changes:
- Introduces external token classification via
--use-classifierflag - Adds configuration file support through
--configflag with YAML format - Implements token trimming feature for display purposes
Reviewed Changes
Copilot reviewed 35 out of 38 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| mg/external_classifier.go | Implements communication protocol with external classification programs |
| mg/tokeniser.go | Updates tokenizer to support external classification and new token roles |
| mg/token.go | Extends token types and precedence handling for external classifiers |
| mg/parser.go | Adapts parser to work with new token classification system |
| mg/config.go | Adds YAML configuration file support with default options |
| cmd/monogram/main.go | Integrates external classification and configuration file handling |
| mg/*_writer.go | Updates all output writers to support token trimming |
| mg/version.go | Version bump to 0.7.1 |
| cmd *exec.Cmd | ||
| stdin io.WriteCloser | ||
| stdout io.ReadCloser | ||
| reader *bufio.Reader | ||
| tokenTexts []string // Track order for batch processing | ||
| classifications map[string]*ExternalTokenClassification // Store results from batch processing | ||
| processed bool // Flag to track if batch processing is complete |
There was a problem hiding this comment.
The ExternalClassifier struct lacks proper documentation for its fields. Public structs should have comprehensive field documentation explaining their purpose and usage patterns.
| cmd *exec.Cmd | |
| stdin io.WriteCloser | |
| stdout io.ReadCloser | |
| reader *bufio.Reader | |
| tokenTexts []string // Track order for batch processing | |
| classifications map[string]*ExternalTokenClassification // Store results from batch processing | |
| processed bool // Flag to track if batch processing is complete | |
| cmd *exec.Cmd // cmd is the running external classification process. | |
| stdin io.WriteCloser // stdin is the input stream to the external process. | |
| stdout io.ReadCloser // stdout is the output stream from the external process. | |
| reader *bufio.Reader // reader wraps stdout for buffered reading of results. | |
| tokenTexts []string // tokenTexts tracks the order of tokens for batch processing. | |
| classifications map[string]*ExternalTokenClassification // classifications stores results from batch processing, keyed by token text. | |
| processed bool // processed indicates whether batch processing is complete. |
| responseBytes, _, err := ec.reader.ReadLine() | ||
| if err != nil { | ||
| // Error will be handled when the channel is closed | ||
| return | ||
| } |
There was a problem hiding this comment.
Error handling is insufficient - the error from ReadLine is silently discarded and the goroutine exits without signaling the error to the main processing logic. This could lead to deadlocks or incomplete processing.
| case "S": // Form-start | ||
| token.Type = Identifier | ||
| token.SubType = IdentifierFormStart | ||
| for _, t := range classification.EndTokens { | ||
| token.SubTokens = append(token.SubTokens, &Token{Type: Identifier, SubType: IdentifierFormEnd, Text: t, Span: Span{-1, -1, -1, -1}, IsMultiLine: false}) | ||
| } |
There was a problem hiding this comment.
The SubTokens field is being used for a different purpose than interpolated strings. Creating tokens with invalid spans (Span{-1, -1, -1, -1}) could cause issues in span-aware operations. Consider using a more explicit field for end token storage.
| token.IsInfixBracket = classification.IsInfixBracket | ||
| token.IsOutfixBracket = classification.IsOutfixBracket | ||
| for _, t := range classification.EndTokens { | ||
| token.SubTokens = append(token.SubTokens, &Token{Type: CloseBracket, SubType: BracketOther, Text: t, Span: Span{-1, -1, -1, -1}, IsMultiLine: false}) | ||
| } |
There was a problem hiding this comment.
Same issue as above - creating tokens with invalid spans for end tokens. This pattern is repeated and should be addressed consistently.
| func isMatchingClosingToken(current *Token, closingType TokenType, closingSubtype uint8, closingTokens []*Token) bool { | ||
| if current.Type != closingType || current.SubType != closingSubtype { | ||
| return false | ||
| } | ||
| if closingSubtype != BracketOther { | ||
| return true | ||
| } | ||
| for _, ct := range closingTokens { | ||
| if current.Text == ct.Text { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The function name suggests it checks for matching closing tokens, but it only checks if the current token matches any of the provided closing tokens. The logic for BracketOther special case needs better documentation to explain why it's treated differently.
| func (c *Config) ApplyConfigDefaults(options *FormatOptions, flagsExplicitlySet map[string]bool) { | ||
| if !flagsExplicitlySet["format"] && c.Format != "" { | ||
| options.Format = c.Format | ||
| } | ||
| if !flagsExplicitlySet["indent"] && c.Indent > 0 { | ||
| options.Indent = c.Indent | ||
| } |
There was a problem hiding this comment.
The condition c.Indent > 0 prevents setting indent to 0 from config file, even when that might be a valid desired value. This should check if the field was explicitly set in the config rather than checking for positive values.
| // Validate use-classifier flag: if explicitly set, it cannot be empty | ||
| if flagsExplicitlySet["use-classifier"] && options.UseClassifier == "" { | ||
| log.Fatalf("Error: --use-classifier flag was provided but no command was specified") | ||
| } |
There was a problem hiding this comment.
This validation logic only covers the case where the flag was explicitly set but empty. Consider also validating that the command exists and is executable when the flag is provided with a value.
| if trimLength >= 2 { | ||
| return value[:trimLength-1] + "…" | ||
| } else if trimLength >= 1 { | ||
| // If trim length is too small for ellipsis, just truncate | ||
| return value[:trimLength] | ||
| } |
There was a problem hiding this comment.
The function doesn't handle the case where len(value) < trimLength properly. It should return the original value when it's already shorter than the trim length. Currently it would panic on value[:trimLength] if trimLength > len(value).
This PR adds the ability to invoke an external program to alter the initial classification of identifiers, operators and brackets, selectively overriding the dynamic discovery of token roles. This additional flexibility means that Monogram can cover more use-cases with more precision i.e. more carefully constrained grammars.
Note: it is very useful to refer to the companion project re-classify that was developed in tandem with this request.