-
Notifications
You must be signed in to change notification settings - Fork 281
Line Reader extension for TurboWarp #2193
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
base: master
Are you sure you want to change the base?
Conversation
I'll give a starting review for best practices |
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.
Generally speaking you should create a new branch in your fork instead of making all commits from the master branch. Having one branch per extension helps organize things a bit better so you don't have file overload in one PR. Looks pretty good, but you'll have to talk to moderators about whether it will get merged.
@@ -0,0 +1,140 @@ | |||
// Name: Line Reader | |||
// ID: linereader |
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 ID should contain your username
// ID: linereader | |
// ID: microBoyLineReader |
// Name: Line Reader | ||
// ID: linereader | ||
// Description: A simple extension for reading newlines and supports special tilde delimiters. Use ~n for UNIX, ~r for Classic Mac OS, and ~z for Windows. | ||
// By: Pear Computer LLC. <https://scratch.mit.edu/users/-Microboy-/> |
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.
Why is this a LLC?
// ID: linereader | ||
// Description: A simple extension for reading newlines and supports special tilde delimiters. Use ~n for UNIX, ~r for Classic Mac OS, and ~z for Windows. | ||
// By: Pear Computer LLC. <https://scratch.mit.edu/users/-Microboy-/> | ||
// License: BSD-2-Clause |
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.
Just out of curiosity wondering why you have special licensing, generally speaking this isn't necessary. Turbowarp recommends Mozilla Public License v2. See CONTRIBUTING.md.
getInfo() { | ||
return { | ||
id: 'linereader', // Unique ID for the extension | ||
name: 'Line Reader', // Name displayed in Scratch |
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 lint is falling because you have a lot of translation errors. You should use Scratch.translate() on the extension name and block texts.
name: 'Line Reader', // Name displayed in Scratch | |
name: Scratch.translate('Line Reader'), // Name displayed in Scratch |
{ | ||
opcode: 'getLine', // Opcode for getting a specific line | ||
blockType: Scratch.BlockType.REPORTER, // It's a reporter block (returns a value) | ||
text: 'line [LINE_NUM] in [TEXT_INPUT]', // The text displayed on the block |
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.
Same here and for the rest of the blocks. Also, I have to ask why you're using so many comments for things that should be obvious? It just distracts from the code
*/ | ||
_processNewlines(text) { | ||
// Ensure the input is treated as a string | ||
let processedText = String(text); |
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.
You shouldn't use JavaScript's native casting functions for block arguments as scratch has numerous odd quirks that could lead to issues. Use Scratch.Cast.toString() or Scratch.Cast.toNumber() for all block inputs.
let processedText = String(text); | |
let processedText = Scratch.Cast.toString(text); |
*/ | ||
getLine(args) { | ||
// Process custom escape sequences into actual newlines | ||
const processedText = this._processNewlines(args.TEXT_INPUT); |
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 should also use casting, assuming it's a string it should probably look like this:
const processedText = this._processNewlines(args.TEXT_INPUT); | |
const processedText = this._processNewlines(Scratch.Cast.toString(args.TEXT_INPUT)); |
// Process custom escape sequences into actual newlines | ||
const processedText = this._processNewlines(args.TEXT_INPUT); | ||
// Ensure the line number is an integer | ||
const lineNum = parseInt(args.LINE_NUM, 10); |
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 argument too and same goes for the rest of the args in every block
class LineReaderExtension { | ||
getInfo() { | ||
return { | ||
id: 'linereader', // Unique ID for the extension |
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 ID should also contain your username and be the same as the one in the header comment
id: 'linereader', // Unique ID for the extension | |
id: 'microBoyLineReader', // Unique ID for the extension |
images/-Microboy-/line-reader.svg
Outdated
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 is showing the SVG as a code instead of an image, don't really know why, maybe it's the review Github Preview problem
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.
for me it works (as long as i have the Preview tab selected).
also the SVG is the wrong size (shouldn't be 480x360 at the very least, probably 600x300). and if you're just embedding a PNG just make the file a PNG itself; that's allowed
!format |
Validate is failing because your image aspect ratio is incorrect, it must be 2:1. Your image is 480x360, the same as the Scratch Stage, but turbowarp should use something like 960x480. Format is failing because you didn't do |
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.
You should not delete CNAME, it's what allows the custom domain name (extensions.turbowarp.org) to work
I also get a lot of AI written vibes from this extension |
That's kinda what I was thinking.... |
Hello. I made a custom extension for TurboWarp that can count the amount of lines in a piece of text, show a certain line of a certain number in text, and can understand UNIX (~n), Classic Mac OS (~r), and Windows (~z) line types. Its unique feature is its custom ~n, ~r, and ~z tilde delimiters that can be useful for writing certain line types easily without rendering issues, but it, importantly, understands the generic Unicode versions too.