Skip to content

Conversation

@floitsch
Copy link

For now mostly syntactic and conventions.

I only changed some of the camlCase to kebab-case.

There are a lot of explicit yield calls. Not clear if all of them
are needed.

Logging with tags is usually the preferred way.

String-interpolation is your friend. (Also calls .stringify
automatically).

Missing toitdocs. They should be there at least for public elements.

A few too many abbreviatons.

For now mostly syntactic and conventions.

I only changed some of the camlCase to kebab-case.

There are a lot of explicit `yield` calls. Not clear if all of them
  are needed.

Logging with `tags` is usually the preferred way.

String-interpolation is your friend. (Also calls `.stringify`
    automatically).

Missing toitdocs. They should be there at least for public elements.

A few too many abbreviatons.
addshore added a commit that referenced this pull request Mar 18, 2025
Extracted from: #1
Co-authored-by: Florian Loitsch <florian@loitsch.com>
addshore added a commit that referenced this pull request Mar 18, 2025
Extracted from: #1
Co-authored-by: Florian Loitsch <florian@loitsch.com>
addshore added a commit that referenced this pull request Mar 18, 2025
Extracted from: #1
Co-authored-by: Florian Loitsch <florian@loitsch.com>
addshore added a commit that referenced this pull request Mar 18, 2025
Extracted from: #1
Co-authored-by: Florian Loitsch <florian@loitsch.com>
addshore added a commit that referenced this pull request Mar 18, 2025
Extracted from: #1
Co-authored-by: Florian Loitsch <florian@loitsch.com>
addshore added a commit that referenced this pull request Mar 18, 2025
@addshore
Copy link
Member

Thanks for the review, I have folded as much of this in as possible for now
Lots of kebabifying, tweaked some of the other things you commented on etc too

@addshore addshore closed this Mar 18, 2025
addshore added a commit that referenced this pull request Mar 21, 2025
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