Fixes #168 Diff not done on real text provided by the AI#172
Conversation
jGauravGupta
force-pushed
the
feature
branch
from
a667fc9 to
656d1fb
Compare
| .map(param -> param.getType().asString()) | ||
| .collect(Collectors.joining(",")) + ")"; | ||
| String signature = buildMethodSignature(method); | ||
| fileMethodSignatures.put(signature, method.toString().length()); |
There was a problem hiding this comment.
I am actually wondering why we cache the length; in any case, is the use of the parsed text correct here?
There was a problem hiding this comment.
By comparing the method body length, it detects whether any changes were suggested by the AI, so the method itself can be compared in the Diff panel instead of the entire class.
There was a problem hiding this comment.
ok got it. shouldn't a hash be more accurate? Of course anyway not on the parsed text.
There was a problem hiding this comment.
ok got it. shouldn't a hash be more accurate? Of course anyway not on the parsed text.
Length is just a quick heuristic, but a hash would be more accurate for detecting actual changes.
| Map<String, Long> fileMethods = new HashMap<>(); | ||
| for (MethodDeclaration method : methods) { | ||
| if (method.getParentNode().isPresent() && method.getParentNode().get() instanceof ObjectCreationExpr) { | ||
| if (isAnonymousInnerMethod(method)) { |
There was a problem hiding this comment.
I've realised now we are not counting and saving the counter, but saving the position. Can you please update the comment too?
| if (edCu.getTypes().isNonEmpty()) { | ||
| methodSignatures.put(edCu.getType(0).getNameAsString(), edCu.toString()); | ||
| } catch (Exception e2) { | ||
| try { |
There was a problem hiding this comment.
What is this case for? Maybe you can add a comment to explain the purpose. However please review how it can be refactored taking into account the try..catch above: if
CompilationUnit aiCu = StaticJavaParser.parse(editorText);
extractClasses(aiCu, editorText, lines, snippetSignatures);
failed above, it would fail here too.
There was a problem hiding this comment.
I think this check was added to handle inner classes or partial code snippets. Anyway, my next focus will be on the patch-based way of suggesting code, so it can auto applied and reverted, specifically #159
| .map(param -> param.getType().asString()) | ||
| .collect(Collectors.joining(",")) + ")"; | ||
| String signature = buildMethodSignature(method); | ||
| fileMethodSignatures.put(signature, method.toString().length()); |
There was a problem hiding this comment.
ok got it. shouldn't a hash be more accurate? Of course anyway not on the parsed text.
| int classesCount = classDecls.size(); | ||
|
|
||
| for (ClassOrInterfaceDeclaration classDecl : classDecls) { | ||
| if (classesCount == 1 || classDecl.isPublic()) { |
There was a problem hiding this comment.
shouldn't this be outside the loop and then you loop only if classesCount is >1? or at least break from the loop?
There was a problem hiding this comment.
Break isn’t needed since only one public class is allowed, and we still need to process other non-public top-level classes.
2 participants
No description provided.