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

Various cleanups #26

Open
wants to merge 29 commits into
base: hubs
Choose a base branch
from
Open

Various cleanups #26

wants to merge 29 commits into from

Conversation

dawsbot
Copy link

@dawsbot dawsbot commented Apr 3, 2024

Peace-of-mind changes:

  1. autorun prettier on each commit
  2. Require the build to still pass on each commit
  3. Explicity type a few of the more complex functions
  4. Expand variable names (using "transaction" instead of "trx")

src/backfill.ts Outdated Show resolved Hide resolved
"prepare": "husky install"
},
"prettier": {
"singleQuote": true,
Copy link
Author

Choose a reason for hiding this comment

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

I just did this to match what the code style was previously @gskril

Checking often for a diff via ./node_modules/.binprettier --write "**/**"

Copy link
Owner

Choose a reason for hiding this comment

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

There is already a .prettierrc file, I'd prefer to stick with that and the existing sorter plugin. I'll open a PR on your branch for some small suggestions including this

package.json Outdated Show resolved Hide resolved
@gskril
Copy link
Owner

gskril commented Apr 13, 2024

Thanks for all your work on this @dawsbot! Working my way through reviewing everything now.

When I wipe the db and do a fresh install:

  • bun install
  • bun kysely:migrate
  • bun dev

I get the following error
image

Have you run into this?

@dawsbot
Copy link
Author

dawsbot commented Apr 13, 2024

I haven't tried a bun install at all @gskril . Notice that there is no .bunx file added to the repo in my commit, so I'm not changing the package manager of this project.

Perhaps that relates? Try a yarn and let me know 🙏

@gskril
Copy link
Owner

gskril commented Apr 14, 2024

Ah, I assumed usage of bun based on changing the start script to bun run src/index.ts. I feel like it makes sense to either fully adopt bun or not at all, no?

src/api/cast.ts Outdated
@@ -8,17 +8,19 @@ import { formatCasts } from '../lib/utils.js'
* Insert casts in the database
* @param msg Hub event in JSON format
*/
export async function insertCasts(msgs: Message[]) {
export async function insertCasts(msgs: Message[]): Promise<void> {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm generally in the camp of inferring return types where possible, in which case this feels overly verbose. Most of the reasoning is here https://www.youtube.com/watch?v=I6V2FkW1ozQ

I can be convinced otherwise though

Copy link
Author

Choose a reason for hiding this comment

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

You've convinced me, I didn't know that from the video! Will update 🫡

@dawsbot
Copy link
Author

dawsbot commented Apr 14, 2024

Ah, I assumed usage of bun based on changing the start script to bun run src/index.ts. I feel like it makes sense to either fully adopt bun or not at all, no?

I've never used their package manager, but honestly your call! I like the ability to opt-out if we start using syntax which is not yet supported by bun. It's not full featured, I've ran into edge-cases prior

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