nextstrain / nextstrain.org

The Nextstrain website
https://nextstrain.org
GNU Affero General Public License v3.0
87 stars 49 forks source link

Don't condition build status on lint result #909

Closed victorlin closed 2 weeks ago

victorlin commented 2 weeks ago

Description of proposed changes

This avoids failure to create Heroku preview apps simply due to linting errors.

Related issue(s)

N/A

References

https://nextjs.org/docs/app/api-reference/next-config-js/eslint

Checklist

victorlin commented 2 weeks ago

Continuing discussion from https://github.com/nextstrain/nextstrain.org/pull/904#issuecomment-2159123924:

This blockage doesn't seem necessary/productive. Attempting removal with #909

it's not necessary, in that i could add lines to disable the couple lint failures -- but then those would need to be removed once #906 lands, and that feels like unproductive churn to me.

(i don't have a strong opinion about the direction #909 moves things in; i think linting being mildly annoying when broken is maybe a good thing…)

As a reviewer, it's nice to have a Heroku preview whenever possible (definitely possible in the case of linting failures). You could make that available with the temporary linting exceptions, but as you've said that's extra churn on your side. Without the Heroku preview there's more churn on the reviewer side – pull latest changes, build/run locally.

The CI lint failure (which isn't visible in #904 but will be with #906) should be apparent enough to prevent unintentional merge.