Skip to content

Clean up SuggestionProvider hierarchy#567

Merged
jpenilla merged 13 commits into2.0.0-devfrom
2/cleanup-suggestionprovider-hierarchy
Dec 17, 2023
Merged

Clean up SuggestionProvider hierarchy#567
jpenilla merged 13 commits into2.0.0-devfrom
2/cleanup-suggestionprovider-hierarchy

Conversation

@jpenilla
Copy link
Copy Markdown
Member

No description provided.

@jpenilla jpenilla added the 2.0.0 label Dec 14, 2023
@jpenilla jpenilla force-pushed the 2/cleanup-suggestionprovider-hierarchy branch from 983eb40 to ab4d2a4 Compare December 14, 2023 19:20
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 14, 2023

Test Results

  80 files  ±0    80 suites  ±0   14s ⏱️ -7s
489 tests ±0  489 ✔️ ±0  0 💤 ±0  0 ±0 
516 runs  ±0  516 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 1dfd3ca. ± Comparison against base commit 8779467.

♻️ This comment has been updated with latest results.

@Machine-Maker
Copy link
Copy Markdown
Contributor

Any use for a static method on SuggestionProvider for some constant suggestions?

Also, a thought I had was about ArgumentParser not extending SuggestionProvider anymore. It just seems like it could be better designed than having ArgumentParser extend it but having implementations implementing a subinterface of SuggestionProvider as well.

What if ArgumentParser had a method that returned SuggestionParser? That way, the correct type of provider, blocking, nonblocking, strings, etc. would just be returned there instead of the overlapping interface implementations on separate types?

@jpenilla jpenilla marked this pull request as ready for review December 17, 2023 19:19
Copy link
Copy Markdown
Member

@broccolai broccolai left a comment

Choose a reason for hiding this comment

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

Haven't looked through all the parsers but LGTM once BlockingSuggestionProvider is un-subclass'd

@jpenilla jpenilla merged commit aba4290 into 2.0.0-dev Dec 17, 2023
@jpenilla jpenilla deleted the 2/cleanup-suggestionprovider-hierarchy branch December 17, 2023 19:38
@Citymonstret Citymonstret mentioned this pull request Dec 17, 2023
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.

4 participants