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

chore: Convert actions to Kotlin #482

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reneleonhardt
Copy link
Contributor

@reneleonhardt reneleonhardt commented Apr 20, 2024

I suggest streamlining all actionPerformed:

  • Send Telemetry if nothing happened?
  • Show dialog if action is not possible?
  • Throw NPE with requireNonNull() or handle null accordingly?

Comment on lines +22 to +26
TelemetryAction.IDE_ACTION
.createActionMessage()
.property("group", null)
.property("action", actionType.name)
.send()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logic changed: Success or Failure telemetry, not both (therefore properties added to error too).
But I can revert if both are needed in case of an error 😅

}

override fun actionPerformed(project: Project, editor: Editor, selectedText: String?) {
if (!selectedText.isNullOrBlank()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed empty to blank

"Staged git diff" to stagedGitDiff,
"New files" to newFilesContent)
.entries.stream()
.filter { !it.value.isNullOrBlank() }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed empty to blank


fun updateState(checkedNode: CheckedTreeNode) {
val fileContent = getNodeFileContent(checkedNode) ?: return
val tokenCount = encodingManager.countTokens(fileContent)
Copy link
Contributor Author

@reneleonhardt reneleonhardt Apr 20, 2024

Choose a reason for hiding this comment

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

I would do nothing if tokenCount == 0 (for example if an error occured) 😅

.send()
}
}
onRefresh.run()
Copy link
Contributor Author

@reneleonhardt reneleonhardt Apr 20, 2024

Choose a reason for hiding this comment

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

Why call onRefresh if event.project == null? 🤔
Why show a dialog if nothing can be done? 😉

}

override fun actionPerformed(event: AnActionEvent) {
if (OverlayUtil.showDeleteConversationDialog() == Messages.YES && event.project != null) {
Copy link
Contributor Author

@reneleonhardt reneleonhardt Apr 20, 2024

Choose a reason for hiding this comment

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

Why show a dialog if event.project == null? 😅

Suggested change
if (OverlayUtil.showDeleteConversationDialog() == Messages.YES && event.project != null) {
if (event.project != null && OverlayUtil.showDeleteConversationDialog() == Messages.YES) {

Comment on lines +44 to +46
TelemetryAction.IDE_ACTION.createActionMessage()
.property("action", ActionType.OPEN_CONVERSATION_IN_EDITOR.name)
.send()
Copy link
Contributor Author

@reneleonhardt reneleonhardt Apr 20, 2024

Choose a reason for hiding this comment

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

Telemetry if nothing happend (e.project == null or getCurrentConversation() == null)? 🤔

@reneleonhardt reneleonhardt force-pushed the convert-actions-to-kotlin branch 2 times, most recently from d00c766 to ed6f96f Compare April 21, 2024 06:05
@reneleonhardt
Copy link
Contributor Author

@carlrobertoh I converted conversations on top of this branch, any chance that you can merge this first?
Another 29 files changed, still 120 to go 😅

@carlrobertoh
Copy link
Owner

I haven't had a chance to review this PR yet, but I'll try to do it sometime early next week. Also, I'll probably do the next release without these refactorings because our test coverage is bad, and I don't want to risk breaking anything.

@reneleonhardt
Copy link
Contributor Author

Sounds good 👍

@reneleonhardt
Copy link
Contributor Author

Speaking of which, please try Llama 3 locally first before releasing 😅

@carlrobertoh
Copy link
Owner

The response seems a bit off. Also, the success callback isn't being triggered once the server is up and running, so it looks like it's booting up forever.

llama3

@carlrobertoh
Copy link
Owner

Was able to fix the response by modifying the prompt and adding a stop token.

llama3_update

@reneleonhardt
Copy link
Contributor Author

Thank you for testing and fixing! Stream was not the problem, begin_of_text isn't needed, but assistant, stopTokens and server hack (?).
Are other models running with msg instead of message, is the hack "official"?

@carlrobertoh
Copy link
Owner

Thank you for testing and fixing! Stream was not the problem, begin_of_text isn't needed, but assistant, stopTokens and server hack (?).

I followed the template format from their official guide. Even if it's not required, I'd still like to have it included unless it breaks something.

Are other models running with msg instead of message, is the hack "official"?

I called it a hack because the way we detect if the server is up and running is through the server logs. I assume they changed their logging structure (from message to msg), and that broke our trigger condition.

@carlrobertoh
Copy link
Owner

Hey, I'm putting this refactoring on pause for a bit until I have run a compatibility comparison between different IDE builds. Most likely, this won't cause any issues, but I just want to be sure, since I'm slowly considering supporting older versions again.

@reneleonhardt reneleonhardt force-pushed the convert-actions-to-kotlin branch 3 times, most recently from 2c68fde to a24a6b6 Compare May 11, 2024 11:16
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.

2 participants