Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
hover show detail feature + minor bug fix #1
base: main
Are you sure you want to change the base?
hover show detail feature + minor bug fix #1
Changes from all commits
d7f83e1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that
utils.remove_special_character
is defined and correctly removes or handles special characters from the word to avoid potential errors or unexpected behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new
hover_definition
function is a significant enhancement for the user experience, providing detailed information on hover. It's important to ensure that the performance impact is minimal, especially for large files. Consider adding performance benchmarks or optimizing the retrieval of hover information if necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure
utils.remove_special_character
method is implemented inlsp_utils.py
as it's critical for the hover feature's functionality and the associated bug fix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling exceptions more specifically to provide clearer error messages or fallbacks, especially for
KeyError
,AtoError
, andAttributeError
within the hover feature implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new hover feature is a significant enhancement for the user experience, providing quick insights into class names and variable values directly in the editor. It's important to ensure that the hover information is accurate and performs well, even in large files or complex projects. Consider adding tests to cover various scenarios, including edge cases where instances or variables might not follow conventional naming or structuring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning a CompletionList for a hover request is incorrect. It should return None instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning lsp.CompletionList in hover_definition is incorrect. It should return None instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning
lsp.CompletionList
in a hover handler is incorrect. It should returnNone
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning an empty completion list for hover requests is not appropriate. Consider returning
None
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning lsp.CompletionList for hover feature is incorrect. It should return None instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of
utils.remove_special_character(word)
is a good addition for handling the minor bug related to brackets in words. Ensure that this utility function is robust and handles all edge cases that might be encountered in different programming languages or file types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of
utils.remove_special_character(word)
is a thoughtful approach to handling edge cases like brackets in words. It's a good practice to document the specific cases this bug fix addresses, ensuring future maintainers understand the context and reasoning behind this change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function remove_special_character is not defined in the provided context. Ensure it is imported or defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
remove_special_character
function is not defined in the provided context. Ensure this function is implemented or imported correctly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling cases where
instance.supers
might be empty.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach to display class names and variable values is clear and concise. However, consider handling edge cases where
instance.supers[0].address
might not exist orassignment[0]
isNone
. Adding error handling or checks could prevent potential runtime errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the Markdown formatting for hover content does not introduce formatting issues in the LSP client. Specifically, verify that newline and bold formatting are displayed as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handling of instance assignments and the display of their values is well-implemented. However, consider the scalability of this approach for instances with a large number of assignments or deeply nested structures. It might be beneficial to limit the depth or quantity of information displayed to keep the hover popup manageable and readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieving class assignments using
atopile.front_end.dizzy.get_layer(class_addr).assignments.get(word, None)
is a neat way to fetch assigned values. Ensure thatdizzy.get_layer
and the assignments dictionary are always up-to-date with the latest changes in the document to maintain accuracy in hover information.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate that
class_assignments.value
is always in a format suitable for direct string conversion and presentation in the hover content.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieving and displaying the value of class assignments is a useful feature. Ensure that the value's string representation is user-friendly and informative. For complex objects, consider how much detail to show, as overly verbose information might overwhelm the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assigning to output_str directly here will overwrite any instance details. Consider appending instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
output_str
should be appended to rather than replaced when handling class assignments to avoid overwriting instance details.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a brief docstring to
remove_special_character
function to explain its purpose and usage, especially how it relates to the hover feature and the handling of brackets in words.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new function
remove_special_character
is a useful addition for sanitizing words by removing specific special characters. It's a good practice to explicitly list which characters are considered special in the function's documentation for clarity and maintainability. Also, consider edge cases where removing these characters might alter the intended meaning or functionality of the word in different contexts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extending the set of characters to remove based on potential future requirements or ensuring this set covers all cases expected by the hover feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a brief docstring to
remove_special_character
function to explain its purpose, especially the context in which it should be used within the LSP utilities.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new function
remove_special_character
effectively strips parentheses and curly braces from the input string. While this is generally useful, consider if there are other special characters that might also need to be removed in the context of your LSP application. Additionally, it might be beneficial to document the specific use case for this function within the codebase to clarify its intended usage and help future contributors understand its role.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a docstring to explain the purpose and usage of this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the value for
hoverProvider
is a boolean (true
) instead of a string to correctly enable hover support according to the VS Code extension API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding 'hoverProvider': true is a great feature enhancement. It's important to ensure that the corresponding implementation is robust and efficiently handles edge cases, especially for complex atopile codebases. Consider adding documentation or tooltips in the UI to inform users about this new feature and how they can make the most of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value for 'hoverProvider' should be a boolean true (without quotes) instead of a string 'true'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value for
hoverProvider
should be a booleantrue
instead of a string. Replace "true" withtrue
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding 'hoverProvider' with a value of 'true' enables hover support, which is a great feature for improving developer experience. However, the value should ideally be a boolean
true
rather than a string "true". Please verify if this implementation aligns with the expected format for VS Code extensions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value for
hoverProvider
should be a boolean (true
) instead of a string ("true"
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value for
hoverProvider
should be a boolean (true
) instead of a string ("true"
). This ensures proper interpretation by VS Code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value for
hoverProvider
should be a boolean. Change "true" to true.