Skip to content
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

Minor typo fixing and code refactoring #2

Merged
merged 4 commits into from
Feb 11, 2024
Merged

Minor typo fixing and code refactoring #2

merged 4 commits into from
Feb 11, 2024

Conversation

tensorush
Copy link
Contributor

@tensorush tensorush commented Feb 11, 2024

Hi! Great work with this TUI library!

I'd like to contribute these fixes:

  • Added standard .gitattributes file for Zig projects.
  • Reworked build.zig a little, hopefully it's a bit clearer. Also, now zig build will run all steps.
  • outer: while in examples was redundant since there's only one loop to break from. switch expressions don't allow breaking from them, so breaking is only for loops, i.e. while and for.
  • When returning a struct instance from a function, the compiler infers the return type from function signature, so instead of return MyType{...}; , it's more idiomatic to write return .{...};.
  • Logging adds a new line by default, so you don't usually need to write \n like here: log.debug("event: {}\r\n", .{event});.

Please tell me if something doesn't make sense, I'll be sure to fix it!

README.md Outdated
| Images (kitty) | ✅ | ✅ | ✅ |
| Images (iterm2) | ❌ | ❌ | ✅ |
| Video | ❌ | ❌ | ✅ |
| Dank | 🆗 | 🆗 | ✅ |
Copy link
Owner

Choose a reason for hiding this comment

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

What formatter did you use? I use prettier for markdown files and it did the formatting of this table automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, let's stick with what prettier outputs. Thanks

src/cell.zig Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

This file should be renamed Cell.zig I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah, will do

@rockorager
Copy link
Owner

Thanks! A couple comments left inline. I appreciate it! Always like to have it be more idiomatic :)

The \r\n in the logs was for when the terminal is in raw mode (IE if you are logging but not redirecting stderr), you'll need the \r to get everything back to column 0. But generally logging should be redirected so I'm good with removing those.

Copy link
Owner

@rockorager rockorager left a comment

Choose a reason for hiding this comment

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

Huh....seems like the file rename isn't here?

@tensorush
Copy link
Contributor Author

tensorush commented Feb 11, 2024

Yeah, had to do a git mv to properly rename it. It's there now.

@rockorager rockorager merged commit 4a463cf into rockorager:main Feb 11, 2024
@rockorager rockorager mentioned this pull request Apr 18, 2024
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.

2 participants