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

nimsuggest: move library code into suggest #892

Merged
merged 5 commits into from
Sep 16, 2023

Conversation

bung87
Copy link
Contributor

@bung87 bung87 commented Sep 12, 2023

Summary

Extract procedures that do not rely on the command-line from the
nimsuggest module to the suggest module.

Details

Changes:

  • procedure findNode was moved and renamed to findTrackedNode
  • procedure symFromInfo was moved and renamed to findTrackedSym
  • the ide command execution part of executeNoHooks was extracted and
    moved into executeCmd

This makes the suggest module more of a reusable component for other
non-nimsuggest tools, e.g. the LSP server.

@saem
Copy link
Collaborator

saem commented Sep 12, 2023

@bung87 does this block #879? If so, can you note blockers/dependencies like that in the Notes for Reviewers section, thanks.

@bung87
Copy link
Contributor Author

bung87 commented Sep 12, 2023

updated.

@saem
Copy link
Collaborator

saem commented Sep 13, 2023

I've done a quick read, but it looks like there was a move plus renames of 2 procedures and the extraction of another procedure.

Were there any other changes that I didn't spot? The extraction of the execueCmd procedure is a highlight that should be covered in the PR description.

compiler/tools/suggest.nim Outdated Show resolved Hide resolved
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

I spotted a few things, but I really like the idea of separating these things out, so we have a general core suggest module and then wrap it with nimsuggest or lsp specific modules to specialize it.

I can help with the PR phrasing, I just need you to provide the core content (what and why).

bung87 and others added 3 commits September 14, 2023 10:33
Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
compiler/tools/suggest.nim Outdated Show resolved Hide resolved
@saem saem added refactor Implementation refactor tool Improvements to non-compiler tooling labels Sep 14, 2023
@saem saem added this to the Tooling milestone Sep 14, 2023
Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
@bung87
Copy link
Contributor Author

bung87 commented Sep 15, 2023

updated

@saem
Copy link
Collaborator

saem commented Sep 15, 2023

updated

Yup, I just need a some time to clean-up the PR message. In the meantime, if you can take another attempt at it based on the recent PR messages I've cleaned up, that'll save me some time, thanks.

@saem saem changed the title refactor nimsuggest, prepare for using in other tools nimsuggest: move library code into suggest Sep 15, 2023
@saem
Copy link
Collaborator

saem commented Sep 15, 2023

@bung87 have a read through the PR title + body

@saem
Copy link
Collaborator

saem commented Sep 15, 2023

/merge

@github-actions
Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

this block #879, as I want rebase and
make cleaner commits.

@chore-runner chore-runner bot added this pull request to the merge queue Sep 15, 2023
Merged via the queue into nim-works:devel with commit a08aa4e Sep 16, 2023
18 checks passed
@bung87 bung87 deleted the refactor-nimsuggest branch September 16, 2023 05:11
bung87 added a commit to bung87/nimskull that referenced this pull request Sep 16, 2023
<!--- The Pull Request (=PR) message is what will get automatically used
as
the commit message when the PR is merged. Make sure that no line is
longer
than 72 characters -->

## Summary

Extract procedures that do not rely on the command-line from the
`nimsuggest` module to the `suggest` module.

## Details

Changes:

- procedure `findNode` was moved and renamed to `findTrackedNode`
- procedure `symFromInfo` was moved and renamed to `findTrackedSym`
- the ide command execution part of `executeNoHooks` was extracted and
  moved into `executeCmd`

This makes the `suggest` module more of a reusable component for other
non-`nimsuggest` tools, e.g. the LSP server.

---------

Co-authored-by: Saem Ghani <saemghani+github@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Implementation refactor tool Improvements to non-compiler tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants