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

fix: display local context #1858

Merged
merged 2 commits into from
Nov 22, 2023
Merged

fix: display local context #1858

merged 2 commits into from
Nov 22, 2023

Conversation

abeatrix
Copy link
Contributor

As discussed with @toolmantim on slack (thread), the context display widget should also include local context except the ones added by the user manually via @

I was considering anything that wasn't explicitly @-included as "enhanced context"

Test plan

Try running the /test command, which use local context only.

BEFORE

Used local context is not showing up in the enhance context widget

image

AFTER

Used local context is included in the context widget

image

@abeatrix abeatrix requested review from toolmantim, a team and kalanchan November 22, 2023 08:54
Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

👍🏼

I've only just spotted that snippets from ${n} files UI language now… "snippets" isn't something we mention anywhere else and it would be good to avoid introducing it here.

Under what condition is that shown? (how does !file.range occur?)

The intention was it would always say n lines from y files, so the user is able to get a sense of just how many lines were used/included in the context (so they can keep an eye on the volume of what's included)

@abeatrix
Copy link
Contributor Author

👍🏼

I've only just spotted that snippets from ${n} files UI language now… "snippets" isn't something we mention anywhere else and it would be good to avoid introducing it here.

Under what condition is that shown? (how does !file.range occur?)

The intention was it would always say n lines from y files, so the user is able to get a sense of just how many lines were used/included in the context (so they can keep an eye on the volume of what's included)

Should I replace snippets with context for files that do not contain line count? (I made the change perviously because chat from older chat history would all display 0 lines from n files before the change.)

@toolmantim
Copy link
Contributor

Should I replace "snippets" with "context" for files that do not contain line count?

{$n} files would be enough. I have to ask though, how could a file not contain a line count? (can we generate one?)

After thinking about it some more, I think "✨" should only display if "Enhanced Context" was checked too.

@abeatrix
Copy link
Contributor Author

Should I replace "snippets" with "context" for files that do not contain line count?

{$n} files would be enough. I have to ask though, how could a file not contain a line count? (can we generate one?)

After thinking about it some more, I think "✨" should only display if "Enhanced Context" was checked too.

cause not all of the sources use range or come from local files, but we can count them manually. We cannot generate them for old chat though so they will show up as ${n} file

@abeatrix
Copy link
Contributor Author

Should I replace "snippets" with "context" for files that do not contain line count?

{$n} files would be enough. I have to ask though, how could a file not contain a line count? (can we generate one?)
After thinking about it some more, I think "✨" should only display if "Enhanced Context" was checked too.

cause not all of the sources use range or come from local files, but we can count them manually. We cannot generate them for old chat though so they will show up as ${n} file

looks like that doesn't work because the local context doesn't use the content field 😓

Updated to show the emoji when enhanced context is used:

image

Will need look into attaching line numbers to different context in a separate issue

@abeatrix abeatrix merged commit 7ed1afb into main Nov 22, 2023
14 checks passed
@abeatrix abeatrix deleted the bee/include-local-context branch November 22, 2023 22:13
DanTup added a commit that referenced this pull request Dec 29, 2023
This test was verifying the user-selected files showed up in the context widget, but this was not intended (see first line of #1858). They were still showing up because the source was not passed through correctly. Now it is, they don't show.
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