-
Notifications
You must be signed in to change notification settings - Fork 0
HDD-1753: typescript + tables #4
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
Conversation
…ause cornerstone shows them even when they're 0
added a makefile just so my dumb brain doesn't forget steps
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.
❌ Changes requested. Reviewed everything up to 2e6d999 in 2 minutes and 8 seconds
More details
- Looked at
1760lines of code in38files - Skipped
3files when reviewing. - Skipped posting
26drafted comments based on config settings.
1. Makefile:1
- Draft comment:
No issues found in Makefile. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. examples/formatting.ts:44
- Draft comment:
Comment on using Colors.GRAY appears explanatory, no changes needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. examples/halloween.ts:1
- Draft comment:
Code looks straightforward; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. examples/image.ts:1
- Draft comment:
Image example properly adds ImageElement; no changes required. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. examples/tables.ts:1
- Draft comment:
Table example code is clear; consider verifying calculated cell widths meet layout needs. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
6. lib/align.ts:1
- Draft comment:
Alignment constants are defined correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. lib/colors.ts:1
- Draft comment:
Color definitions are consistent with the RGB class usage; nothing to change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. lib/elements/element.ts:1
- Draft comment:
Abstract Element class in TS implementation is correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. lib/elements/group.ts:30
- Draft comment:
GroupElement’s async handling is clear; use of async.parallel is acceptable. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
10. lib/elements/image.ts:19
- Draft comment:
ImageElement reads file and converts to hex as expected; error handling is adequate. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. lib/elements/table.ts:66
- Draft comment:
TableElement correctly computes cell widths and applies border formatting; review default width logic to ensure layout correctness. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
12. lib/elements/text.ts:1
- Draft comment:
TextElement wraps text using format; implementation is clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
13. lib/fonts.ts:1
- Draft comment:
Font definitions are complete and typed as constants. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
14. lib/format.ts:80
- Draft comment:
Wrap function implementation simulates toggling formatting; consider using arrow functions for consistency. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
15. lib/index.ts:1
- Draft comment:
Index file properly re-exports modules; no issues. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
16. lib/language.ts:1
- Draft comment:
Language constants defined correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
17. lib/orientation.ts:1
- Draft comment:
Orientation constants are clear; no changes needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
18. lib/rgb.ts:1
- Draft comment:
RGB class is straightforward; no concerns. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
19. lib/rtf-utils.ts:3
- Draft comment:
Extending the String prototype for replaceAll is acceptable but may require polyfill in older runtimes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
20. lib/rtf.ts:125
- Draft comment:
Consider using arrow functions in async callbacks to avoid capturing 'self', improving readability. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
21. package.json:1
- Draft comment:
package.json updated for TypeScript build; dependencies and scripts appear correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
22. lib/format.ts:92
- Draft comment:
The wrap function appends '0' after the control word. Please ensure this complies with the RTF specification or document the rationale. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure compliance with a specification or document the rationale, which is similar to asking for confirmation or explanation. This violates the rule against asking the PR author to confirm their intention or explain their changes.
23. lib/rtf-utils.ts:3
- Draft comment:
Ensure that String.prototype.replaceAll is available in your target environment or provide a polyfill to avoid runtime errors. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure compatibility with the target environment, which is against the rules. It doesn't provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
24. Makefile:8
- Draft comment:
The file is missing a newline at the end. Although this is a minor stylistic issue, it's best practice to end files with a newline to avoid potential issues with some tools. - Reason this comment was not posted:
Marked as duplicate.
25. examples/tables.ts:19
- Draft comment:
Typo: The table row contains the string 'Sproket', which might be a misspelling of 'Sprocket'. Please double-check and correct it if needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
Since this is an example file, correctness of the content matters as it serves as documentation. However, 'Sproket' could theoretically be an intentional product name. The comment is phrased as a suggestion to "double-check" which violates our rule about not asking authors to verify things. The comment should either definitively point out an error or be removed.
What if 'Sproket' is an intentional spelling for a fictional product in this example? Examples sometimes use made-up names deliberately.
While that's possible, in an example file it's more likely this is an actual typo that should be fixed, as using a misspelled word could confuse readers.
The comment should be deleted because it asks the author to "double-check" rather than making a definitive statement, violating our commenting rules.
26. lib/elements/table.ts:105
- Draft comment:
Typographical error: In the comment starting at line 104, change 'and not the just text within the cell' to 'and not just the text within the cell' to improve clarity. - 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%
While the comment is technically correct about the grammatical error, it's an extremely minor issue in a code comment that doesn't affect functionality or code understanding. The meaning is clear even with the typo. According to the rules, we should not make purely informative comments or comments that are unimportant. This seems to fall into that category.
The typo could potentially confuse non-native English speakers. Also, maintaining high quality even in comments shows attention to detail.
While quality is important, this is an extremely minor grammatical issue that doesn't impact code understanding or maintainability. The rules explicitly state not to make unimportant comments.
Delete this comment as it's too minor and doesn't affect code quality or understanding. It violates the rule about not making unimportant comments.
Workflow ID: wflow_f9PVgUqOYf34VSjJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
❌ Changes requested. Incremental review on af40307 in 1 minute and 37 seconds
More details
- Looked at
103lines of code in5files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. Makefile:8
- Draft comment:
Ensure the file ends with a newline for POSIX compliance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. examples/tables.ts:24
- Draft comment:
The addition of 'myDoc.addTable(table2)' is correct, but update comments if needed to reflect that table2 is added explicitly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. lib/rtf.ts:70
- Draft comment:
Using 'CommandElement' instead of a plain object improves type safety—good update. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. Makefile:8
- Draft comment:
Ensure consistent trailing newline per project style. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. examples/tables.ts:21
- Draft comment:
Verify that adding table2 via myDoc.addTable is intentional and preserves the desired table order. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. lib/elements/command.ts:9
- Draft comment:
Consider replacing the 'any' type for the 'format' parameter with a more specific type for better type safety. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. lib/elements/image.ts:4
- Draft comment:
Use 'const' instead of 'var' for requiring modules to prevent unintended reassignments. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. lib/rtf.ts:66
- Draft comment:
Cache the result of _groupIndex to avoid duplicate calls and improve clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
9. lib/elements/command.ts:4
- Draft comment:
Consider changing 'rtf command' to 'RTF command' in the comment on line 4 for better consistency with common acronym usage. - 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%
This is purely a documentation style suggestion. While technically correct that RTF is typically capitalized as an acronym, this kind of minor comment about documentation style doesn't meet our bar for "clearly a code change required." It's more of a nitpick that doesn't affect functionality or code quality in any meaningful way.
The consistent capitalization of acronyms could be considered part of code quality and maintainability standards. Some teams have strict documentation style guides.
While documentation consistency is good, our rules explicitly state not to make purely informative comments or obvious/unimportant changes. This falls into that category.
Delete this comment as it's a minor documentation style suggestion that doesn't meet our threshold for required code changes.
Workflow ID: wflow_fuvwKU4jRKbsf3fy
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
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.
👍 Looks good to me! Incremental review on ce0631e in 41 seconds
More details
- Looked at
12lines of code in1files - Skipped
1files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. package.json:3
- Draft comment:
Version bump to 0.1.2 looks appropriate. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. package.json:7
- Draft comment:
New metadata (author, scripts, repository, keywords, license, dependencies) improves project clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. package.json:3
- Draft comment:
Version bumped from 0.1.1 to 0.1.2. Confirm that this follows the appropriate semver guidelines. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. package.json:8
- Draft comment:
Added a build script using 'tsc'. Consider expanding scripts (e.g. test, lint) if applicable. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. package.json:11
- Draft comment:
Repository, keywords, and license fields were added. Verify that the repository URL and metadata are correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. package.json:21
- Draft comment:
New dependencies and devDependencies added. Ensure these versions are current and needed for the TS rebuild. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_5WKP7kvaA4blflbh
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
davemilller
left a comment
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.
nice
rebuilt it in typescript
added proper table support
Important
Convert codebase to TypeScript and add table support in RTF library.
libandexamplesto TypeScript, includingalign.ts,colors.ts,elements/*,format.ts,fonts.ts,language.ts,orientation.ts,rgb.ts,rtf-utils.ts,rtf.ts.package.jsonto include TypeScript build script and type definitions.TableElementclass inelements/table.tsfor handling table structures in RTF documents.examples/tables.tsto demonstrate table usage.Makefilefor local development setup with npm commands.package.jsondependencies and devDependencies for TypeScript and related tools.This description was created by
for ce0631e. It will automatically update as commits are pushed.