gadicc / node-yahoo-finance2

Unofficial API for Yahoo Finance
https://www.npmjs.com/package/yahoo-finance2
MIT License
353 stars 58 forks source link

chore(other): added prettier write command to build #679

Closed z3nful closed 9 months ago

z3nful commented 9 months ago

Changes

Type

Comments/notes

This should make it easier to create PRs. I left yarn generateSchema out of the build script as I am still not 100% sure where it actually goes in the build process lol

gadicc commented 9 months ago

Hey, thanks. Submitting PRs only to get a fail on prettier not having been run is indeed frustrating. But this isn't really the right place to do it... prettier only affects source files, the built and published files are the transpiled output from tsc, so this has no effect on them, and there's also no guarantee that someone will build the package before submitting a PR (e.g. all my updates are usually done through TDD).

The "correct" place for this (and for generateSchema !) is a pre-commit hook, as mentioned in e.g. https://prettier.io/docs/en/precommit.html. At one stage I had that set up, but, it really slowed down commits, which became very frustrating so I eventually removed it. Perhaps longer term we could have some of our own code to track which files have been changed - and only run prettier on them - and if they could affect the schema - and only then check the schema. That's probably the best of all worlds.

z3nful commented 9 months ago

I see what you mean, and I should do a bit more research on how the build flow goes outside of my local setup as well. I can imagine how having either prettier or generateSchema running against each commit could be taxing against the system, and I'm not sure why I didn't realize that before submitting this PR lol

I'm going to close this to clear up the list, but I'll keep it on my mind as a learning exercise for when I have time :)