Skip to content

Conversation

prusnak
Copy link
Collaborator

@prusnak prusnak commented Apr 12, 2023

No description provided.

j-f1
j-f1 previously approved these changes Apr 12, 2023
@slaren
Copy link
Member

slaren commented Apr 12, 2023

The changes to .editorconfig look good, but I am not sure that it is a good idea to add a space to these prompts, since it can mess with the tokenization of the next word. Instead, we have --in-prefix " " for that purpose. reason-act.sh already has --in-prefix " ", and probably this should be added to chat.sh as well.

@j-f1 j-f1 dismissed their stale review April 12, 2023 18:21

Agree with @slaren — the space should be removed.

@prusnak
Copy link
Collaborator Author

prusnak commented Apr 12, 2023

Okay, so the space should be removed. What about newline at the end of prompt files? Should it be kept or removed too?

@slaren
Copy link
Member

slaren commented Apr 12, 2023

It depends on the prompt but in these cases it should be removed, adding a new line at the end of a prompt should be a deliberate choice and not something forced by the editor.

@prusnak
Copy link
Collaborator Author

prusnak commented Apr 12, 2023

It depends on the prompt but in these cases it should be removed, adding a new line at the end of a prompt should be a deliberate choice and not something forced by the editor.

I am asking about the two particular prompt files I touched in this PR.

@slaren
Copy link
Member

slaren commented Apr 12, 2023

I don't think these prompts should end in a new line.

@prusnak prusnak force-pushed the editorconfig-prompts branch from bc6a5ce to 9b52fc3 Compare April 13, 2023 08:52
@prusnak
Copy link
Collaborator Author

prusnak commented Apr 13, 2023

Thanks @slaren!

Updated the PR to reflect review comments. The files prompts/chat-with-bob.txt and prompts/reason-act.txt do not contain final newlines now. See 9b52fc3

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.

3 participants