codefordallas / codefordallas.github.io

Code for Dallas Website
7 stars 10 forks source link

Add ci #49

Closed jhoover4 closed 11 months ago

jhoover4 commented 1 year ago

Adding CI functionality, prettier, and eslint. For this to work we have to move everything to root so the project is not /src.

Prettier

Prettier is a code formatter used by the javascript community to automatically enforce one code style so there is no arguing about "spaces/tabs" or single quotes or double quotes, etc. It leaves what we call "bike shedding" (arguing about dumb pointless things) at the door. See https://prettier.io/ for more details.

Eslint

Eslint is used to check code for common errors and automatically correct them. This keeps it easy for reviewers to focus on more complex issues and helps you quickly fix issues. See https://eslint.org/ for more details. We're using the React extension here, which is a common extension for react apps.

Continuous Integration

Continous Integration is what developers call the automatic process of linting (checking code for style errors or erros in general), testing, and other checks BEFORE your branch is merged into master (integrated). This is done usually through platforms that are running "jobs" for these various tasks. See https://docs.gitlab.com/ee/ci/introduction/ for a good overview. Here we're using https://docs.github.com/en/actions/using-workflows as our CI runner to execute these tasks for us.

ProsperousHeart commented 1 year ago

@jhoover4 - the files have been moved as of #53. With public folder being removed from .gitignore file, that folder was also uploaded to the repo. So the error you were experiencing should no longer occur. (In the future, please let us know ASAP if you run into issues like that so we can avoid confusion, misunderstandings, issues for other volunteers, etc.)

It looks like this change introduced some conflicts to this PR - likely because in this PR the files appear to have been cut and pasted instead of moved. (See files changed.)

If it is easier, feel free to close this PR and create 2 new ones for:

Otherwise, you can keep this one open for CI but please pull from v1-mvp branch, remove the linting, and create a new PR for linting. As discussed in Discord, we work on one issue or enhancement per PR request.

Thank you!

jhoover4 commented 1 year ago

@ProsperousHeart I did let you know ASAP, I'm the one who found it and fixed it. I'm confused why PRs aren't sufficient for communication here.

The merge conflict is just there because we pulled in the other PR. All you need to do is merge locally and push up, which I did.

I'm not going to make a whole other PR for linting/CI. This is an operation that is being done through the CI and makes sense as one PR. Why are we introducing more work and overhead here when the work is already done? This is already a very small PR.

ProsperousHeart commented 1 year ago

with #57 now complete, this one should be good to go for reintegration and final check before merging

thank you again for working with us through this

ProsperousHeart commented 1 year ago

I also saw the package-lock file being the only package file updated. Any idea to that @jhoover4 ?

jhoover4 commented 11 months ago

I also saw the package-lock file being the only package file updated. Any idea to that @jhoover4 ?

Probably because I didnt commit it before and things have updated 🤷. It should be fine after this