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

Implement .copilotignore support to ignore files #169

Merged
merged 29 commits into from
Jun 26, 2024
Merged

Conversation

TerminalFi
Copy link
Owner

No description provided.

@jfcherng
Copy link
Collaborator

jfcherng commented Jun 19, 2024

As per #98 (comment), I think we should follow .copilotignore rather than invent our own.

As per https://github.com/orgs/community/discussions/10305#discussioncomment-6069843, it seems that .copilotignore already works in VSCode.

LSP-copilot.sublime-settings Outdated Show resolved Hide resolved
@timfjord
Copy link
Collaborator

It looks like this .copilotignore is basically a list of patterns to ignore in a format similar to the one introduced in the copilot_ignore setting.
So maybe we could rely on the .copilotignore by simply splitting it into lines and treating it like the copilot_ignore setting in the PR (we just need to think how to cache it properly)?

@TerminalFi TerminalFi changed the title EARLY implement copilot_ignore setting to ignore files EARLY implement .copilotignore support to ignore files Jun 19, 2024
@TerminalFi
Copy link
Owner Author

@timfjord @jfcherng some progress. Probably worth you all testing. Still need to think about proper way to maybe log pattern matching and if this is global

Signed-off-by: Jack Cherng <[email protected]>
"wcmatch" depends on "bracex" and PC v4 won't resolve dep's dep.

Signed-off-by: Jack Cherng <[email protected]>
@jfcherng jfcherng added the enhancement New feature or request label Jun 21, 2024
@TerminalFi
Copy link
Owner Author

@jfcherng @timfjord I think we only need to cover somehow detecting new folders added to a window. And disabling text commands if file is ignored.

But does the decorator approach work well here?

@TerminalFi
Copy link
Owner Author

We could combine these decorators to maybe

def _must_be_active_view_not_ignored(*, failed_return: Any = None) -> Callable[[T_Callable], T_Callable]:
    def decorator(func: T_Callable) -> T_Callable:
        @wraps(func)
        def wrapped(self: Any, *arg, **kwargs) -> Any:
            if is_active_view(self.view) and not CopilotIgnore(self.view.window()).trigger(self.view):
                return func(self, *arg, **kwargs)
            return failed_return

        return cast(T_Callable, wrapped)

    return decorator

@jfcherng
Copy link
Collaborator

jfcherng commented Jun 21, 2024

We could combine these decorators to maybe

It seems to make sense because .copilotignore works for all copilot features

@timfjord
Copy link
Collaborator

Maybe to avoid dealing with edge cases, we could add a separate command to manually reload the .copilotignore file

@TerminalFi TerminalFi marked this pull request as ready for review June 21, 2024 19:52
@TerminalFi TerminalFi changed the title EARLY implement .copilotignore support to ignore files Implement .copilotignore support to ignore files Jun 21, 2024
@TerminalFi
Copy link
Owner Author

TerminalFi commented Jun 21, 2024

@jfcherng I can't seem to run ci-fix ?

make ci-fix
========== fix: ruff (lint) ==========
ruff check --fix .
All checks passed!
========== fix: ruff (format) ==========
ruff format .
14 files left unchanged

@TerminalFi
Copy link
Owner Author

I was also thinking about updating statusbar LSP-copilot (ignored) if a file is ignored ? Thoughts?

@jfcherng
Copy link
Collaborator

jfcherng commented Jun 21, 2024

@jfcherng I can't seem to run ci-fix ?

make ci-fix
========== fix: ruff (lint) ==========
ruff check --fix .
All checks passed!
========== fix: ruff (format) ==========
ruff format .
14 files left unchanged

it runs and nothing needs to be changed. it won't fix mypy issues for you.

@jfcherng
Copy link
Collaborator

jfcherng commented Jun 21, 2024

Maybe to avoid dealing with edge cases, we could add a separate command to manually reload the .copilotignore file

I thought that we can have a ViewEventListener for view activation. When a view is activated (i.e., focused), read .copilotignore file and set is_copilot_ignored attribute to the view. Reading the file shouldn't take too much time.

@jfcherng
Copy link
Collaborator

I was also thinking about updating statusbar LSP-copilot (ignored) if a file is ignored ? Thoughts?

Following up, we can just read is_copilot_ignored attribute of the activated view. status_text is now a jinja2 template. It should be quite flexible.

@TerminalFi
Copy link
Owner Author

Maybe to avoid dealing with edge cases, we could add a separate command to manually reload the .copilotignore file

I thought that we can have a ViewEventListener for view activation. When a view is activated (focused), read .copilotignore file and set is_copilot_ignored attribute to the view. Reading the file shouldn't takes to much time.

I don't know that we need to read file per view. Probably need to update to ensure the following

  1. File is read on new window
  2. File is read when file is saved that name is .copilotignore
  3. File is read when new folder added to window

Can 3. be achieved?

@jfcherng
Copy link
Collaborator

I just made a renaming 8308bc0

@jfcherng
Copy link
Collaborator

File is read when new folder added to window

This seems to be an issue if we go the watcher way.

@jfcherng
Copy link
Collaborator

Hmm.. users can drag sidebar folder to change to order of workspace folders. I guess this will break the current implementation?

@TerminalFi
Copy link
Owner Author

Hmm.. users can drag sidebar folder to change to order of workspace folders. I guess this will break the current implementation?

Why does order matter ? We store the folder prefix with its patterns. ?

@TerminalFi
Copy link
Owner Author

File is read when new folder added to window

This seems to be an issue if we go the watcher way.

Yeah, we need ST support to fix all
Caveats

plugin/ui/completion.py Outdated Show resolved Hide resolved
@jfcherng
Copy link
Collaborator

I got the following error in console.

ValueError: ('is_enabled must return a bool', <LSP-copilot.plugin.commands.CopilotSignOutCommand object at 0x000002C0BB580EE0>)

@TerminalFi
Copy link
Owner Author

TerminalFi commented Jun 25, 2024

I got the following error in console.

ValueError: ('is_enabled must return a bool', <LSP-copilot.plugin.commands.CopilotSignOutCommand object at 0x000002C0BB580EE0>)

Yes, that is because of

    @_must_be_active_view_not_ignored()
    @_provide_plugin_session(failed_return=False)
    def is_enabled(self, plugin: CopilotPlugin, session: Session) -> bool:  # type: ignore
        return self._can_meet_requirement(session)

The decorator can be used @_must_be_active_view_not_ignored() I don't think.

We will need to disable certain text commands manually if ignored instead of a blanket decorator on all text commands

@TerminalFi TerminalFi merged commit c9ffa2c into master Jun 26, 2024
2 checks passed
@jfcherng jfcherng deleted the impl/copilotIgnore branch June 26, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants