Makes ap chat prettier - #2
Conversation
Implement to_hash method that provides a clean, readable format when using amazing_print. Features include: - Dynamic inclusion of all instance variables - Security: excludes @api_key and @client - Truncates long message content to 80 chars - Skips nil values for cleaner output
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 6f9388f in 1 minute and 50 seconds. Click for details.
- Reviewed
40lines of code in1files - Skipped
1files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/chat.rb:185
- Draft comment:
Unnecessary string interpolation when setting the hash key. Use self.class.name directly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_zMT4pbkgHe90Qb9E
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| value = value.map do |msg| | ||
| truncated_msg = msg.dup | ||
| if msg[:content].is_a?(String) && msg[:content].length > 80 | ||
| truncated_msg[:content] = msg[:content][0..77] + "..." |
There was a problem hiding this comment.
Off-by-one error in truncation: using [0..77] yields 78 characters, so adding '...' makes 81 characters in total. Adjust the slice (e.g., [0..76]) to meet the 80-character limit.
| truncated_msg[:content] = msg[:content][0..77] + "..." | |
| truncated_msg[:content] = msg[:content][0..76] + "..." |
Implement to_hash method that dynamically includes all instance variables for clean formatting. This makes Response objects display with proper indentation when nested within Chat objects.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed e541693 in 1 minute and 21 seconds. Click for details.
- Reviewed
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/response.rb:15
- Draft comment:
Security: The loop exposes all instance variables. Exclude sensitive ones (e.g., @api_key, @client) as mentioned in the PR. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment makes a speculative security suggestion without evidence. The class only has documented instance variables @id, @model, @Usage, and @total_tokens - none of which are sensitive. There's no indication in the code that @api_key or @client exist. The comment seems to be making assumptions about variables that aren't present. Maybe there are parent classes or mixins that add these sensitive variables that we can't see in this diff. Maybe the PR description mentions these variables. Even if those variables exist somewhere else, we should follow the rule to ignore cross-file issues and only focus on what we can see in this diff. We need strong evidence to keep a comment. Delete the comment because it makes assumptions about sensitive variables that aren't shown to exist in this class, and we don't have strong evidence to support the security concern.
2. lib/ai/response.rb:13
- Draft comment:
Consider storing self.class.name in a local variable to avoid repeated string interpolation for cleaner code. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the suggestion would technically reduce duplicate code, the impact is minimal. The code is already quite readable and the duplication is only two lines apart. String interpolation of self.class.name is not an expensive operation that would cause performance issues. This feels like an overly nitpicky suggestion that doesn't meaningfully improve the code. The suggestion could improve code maintainability slightly since changes to the class name reference would only need to be made in one place. It might also make the code marginally more efficient. The benefits are too minor to justify the comment. The current code is clear and the suggested change wouldn't meaningfully improve readability or performance. This comment should be removed as it suggests a minor optimization that doesn't provide significant value and falls into the category of being too obvious/unimportant.
3. lib/ai/response.rb:22
- Draft comment:
Style: Please add a newline at the end of the file to adhere to common formatting conventions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While missing newlines at EOF can cause minor issues with some tools and is generally considered good practice, this is a very minor issue that would likely be caught by linters or formatters. It's not a logic issue and doesn't affect functionality. Many IDEs automatically add these newlines on save. The comment is technically correct, but is it important enough to warrant a PR comment? This could be handled automatically by tooling. While it could be handled by tooling, if there's no automated formatter in place, this is still a valid style issue that's worth fixing for consistency. The comment should be deleted as it's too minor of an issue and would be better handled by automated tooling rather than manual review comments.
Workflow ID: wflow_daRXfjrOfmCulh8M
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Replace hash-based formatting with object notation for better clarity. AI::Chat and AI::Response now display as proper objects with instance variables using @var: syntax. Features include: - Object notation (#<AI::Chat ...>) instead of hash format - Instance variables shown with colons (@messages:, @model:, etc) - Automatic content truncation for long messages (80 chars) - Sensitive data filtering (@api_key, @client hidden) - Proper indentation for nested objects and arrays This makes the output more intuitive for beginners who expect to see objects rather than hash representations.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 88ccca5 in 2 minutes and 2 seconds. Click for details.
- Reviewed
211lines of code in5files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai-chat.rb:3
- Draft comment:
Optional loading of amazing_print is handled well. The rescue for LoadError gracefully skips custom formatting when the gem isn't available. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. lib/ai/amazing_print.rb:5
- Draft comment:
Using alias_method to override 'cast' works, but consider using prepend for a less invasive approach if possible. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. lib/ai/amazing_print.rb:38
- Draft comment:
Using msg.dup assumes the message hash is mutable; consider handling cases where the object might be frozen to avoid potential errors. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. lib/ai/amazing_print.rb:39
- Draft comment:
Hardcoding a maximum length of 80 characters for message content truncation may limit flexibility. Consider parameterizing the limit. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. lib/ai/amazing_print.rb:81
- Draft comment:
Ensure the file ends with a newline for POSIX compliance. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. lib/ai/chat.rb:147
- Draft comment:
Removal of the to_hash method from Chat may be a breaking change if external code relies on it for serialization. Ensure documentation and tests reflect this change. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The to_hash method seems to be a utility method for debugging/serialization. However, there's no clear evidence that this is part of the public API or that external code relies on it. The method appears to be more of an internal utility, especially given its special handling of @api_key and truncation of messages. The class already has inspect for debugging purposes. I might be underestimating the importance of this method - it could be used by external code for persistence or serialization purposes. The careful handling of sensitive data suggests it might be intentionally designed for external use. Without clear evidence that this method is part of the public API or documentation showing its intended external use, we should assume it's an internal utility. The presence of inspect() suggests this was for debugging. The comment should be removed as it's speculative about external code usage without clear evidence that the method was part of the public API.
7. lib/ai/response.rb:9
- Draft comment:
Removal of the to_hash method from Response may affect consumers that rely on it for object serialization. Confirm that this breaking change is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is asking for confirmation about intention, which violates our rules. It's speculative about "may affect consumers". We don't have visibility into whether this method is actually used by consumers. The comment isn't suggesting a specific fix, just asking for confirmation. The method removal could genuinely be a breaking change. Serialization methods are often important parts of a public API. While the concern might be valid, the comment violates multiple rules: it asks for confirmation and is speculative. If this was a critical method, there would likely be failing tests or build issues. Delete the comment because it violates our rules by being speculative and asking for confirmation, rather than pointing out a definite issue that needs fixing.
Workflow ID: wflow_rTIgXHLVHltTnG9f
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
This is neat. I didn't realize how much you could do with amazing print. Did you mention previously that you didn't like including the |
Exclude temporary demo scripts from version control
There was a problem hiding this comment.
Skipped PR review on 4247e43 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
Implement pretty_print methods for AI::Chat and AI::Response to provide clean output with Ruby's built-in pp command. Features: - Truncates long message content to 80 characters - Hides sensitive instance variables (@api_key, @client) - Displays objects with proper notation and indentation - Works alongside amazing_print for maximum flexibility
I think I'm okay with it in this context. And, if we start adding an |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 652cf88 in 2 minutes and 8 seconds. Click for details.
- Reviewed
63lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. lib/ai/chat.rb:192
- Draft comment:
Consider adjusting the truncation range: if the intent is to display a total of 80 characters (including the ellipsis), then using [0..76] might be more appropriate than [0..77], since the current logic yields 81 characters. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is technically correct - the current code will produce 81 characters (78 from the slice + 3 from "..."). However, this is a display formatting detail in a debug/development helper method (pretty_print). The exact character count being 80 vs 81 has no functional impact. The current code still achieves its goal of reasonable length truncation for readability. The comment is mathematically accurate. In some contexts, precise string lengths could matter for alignment or display purposes. While technically correct, this is an extremely minor detail in a debug printing method where the exact character count doesn't impact functionality or usability. This comment should be removed as it suggests a change that is too minor to be worth addressing, especially for a debug/development helper method.
2. lib/ai/response.rb:15
- Draft comment:
For consistency and cleaner output, consider using q.seplist for iterating over instance variables in pretty_print instead of manually handling commas with each_with_index. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_KCLBQUuzruhvvTU7
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
|
||
| # Show other instance variables (except sensitive ones) | ||
| skip_vars = [:@messages, :@api_key, :@client] | ||
| instance_variables.sort.each do |var| |
There was a problem hiding this comment.
For cleaner formatting, consider filtering out sensitive instance variables (e.g. using reject) and using q.seplist to automatically handle comma separation instead of manual iteration with each index.
|
#10 should make this a bit simpler, if we decide to go that way. |
|
@raghubetina aside from addressing merge conflicts and code quality, is this PR feature complete? |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6e63430 in 1 minute and 18 seconds. Click for details.
- Reviewed
54lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. README.md:600
- Draft comment:
Use hash accessor ([:id]) instead of method call to match the new response object structure. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. examples/10_additional_patterns.rb:82
- Draft comment:
Good use of .dig(:response, :id) for safe access to the response id. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply praises the use of a method without offering any constructive criticism or questions.
3. examples/12_image_generation.rb:25
- Draft comment:
Update to hash access ([:id] and [:images]) aligns with new response format. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining why a change was made. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. It violates the rule against making purely informative comments.
4. spec/integration/ai_chat_integration_spec.rb:257
- Draft comment:
Refactor test to use hash accessor ([:id]) for consistency with response object. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_E0K3v3GUmJPr2XV2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Makes
ap chatandpp chatprettier.Changes
Amazing Print Integration
lib/ai/amazing_print.rb) that displays objects with proper notation instead of hash-like output.#<AI::Chat ...>with instance variables shown using@var:syntax.amazing_printis available.Pretty Print Support
pretty_printmethods for bothAI::ChatandAI::Responseclasses.ppcommand out of the box.Features
@api_key,@client) are hidden from output.Example Output
Before:
#<AI::Chat @messages=[{:role=>"system", :content=>"You are a helpful assistant"}, {:role=>"user", :content=>"What's the best pizza in Chicago?"}, {:role=>"assistant", :content=>"Hark! In fair Chicago..."}] @model="gpt-4.1-nano" @schema=nil @reasoning_effort=nil>After (with
ap):After with
pp:Usage
Simply use
pporapas you normally would:Other Changes
.gitignoreto exclude temporary demo scripts.Gemfile.lock.Important
Enhances
AI::ChatandAI::Responseoutput formatting withamazing_printandpretty_print, including content truncation and sensitive data hiding.lib/ai/amazing_print.rbfor custom formatting ofAI::Chatobjects withamazing_print.#<AI::Chat ...>with@var:syntax for instance variables.amazing_printis available.pretty_printinAI::ChatandAI::Responsefor use with Ruby'spp.@api_key,@client)..gitignoreto exclude temporary demo scripts.Gemfile.lock.This description was created by
for 6e63430. You can customize this summary. It will automatically update as commits are pushed.