Skip to content

Conversation

jhoover4
Copy link

@jhoover4 jhoover4 commented Jun 7, 2023

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.

@jhoover4 jhoover4 changed the base branch from master to v1-mvp June 7, 2023 03:03
@ProsperousHeart
Copy link
Member

@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:

  • linting
  • CI

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!

@ProsperousHeart ProsperousHeart mentioned this pull request Jun 9, 2023
@ProsperousHeart ProsperousHeart added enhancement Not a requirement - enhances or improves project requirements automated testing This is related to the testing files for React.js labels Jun 9, 2023
Copy link
Member

@ProsperousHeart ProsperousHeart left a comment

Choose a reason for hiding this comment

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

Please see notes / discussions / responses, including but not limited to (since apparently I can't link to some of my own comments/reviews):

Also curious why you used the 2 specific npm packages listed instead of:

  • eslint (over 32M weekly downloads and last updated June 2, 2023)
  • prettier (over 29M weekly downloads with last update 2 months ago)

Copy link
Member

Choose a reason for hiding this comment

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

updated for linting

Copy link
Member

Choose a reason for hiding this comment

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

updated for linting - approved

Copy link
Member

Choose a reason for hiding this comment

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

updated for linting - approved

Copy link
Member

Choose a reason for hiding this comment

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

this was moved out oy my-app into the main app folder as per here

Copy link
Member

Choose a reason for hiding this comment

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

moved to from my-app to main app folder (parent) for #50 - approved

Copy link
Member

Choose a reason for hiding this comment

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

moved to from my-app to main app folder (parent) for #50 - approved

@jhoover4
Copy link
Author

@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
Copy link
Member

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

jordan.hoover added 2 commits July 21, 2023 16:42
# Conflicts:
#	.eslintrc.js
#	.gitignore
#	docs/react-README.md
#	package-lock.json
#	package.json
#	src/App.test.js
#	src/components/About/About.js
#	src/components/Header/header.html
#	src/components/Logo/Logo-ORIGINAL.jsx
#	src/components/Mission/index.jsx
#	src/components/Nav/nav-BASE.js
#	src/components/Nav/nav.js
#	src/components/Projects/chatGPT-Projects.html
@jhoover4 jhoover4 force-pushed the add_ci branch 2 times, most recently from 5e235e6 to 88c7cc8 Compare July 21, 2023 23:57
Copy link
Member

@ProsperousHeart ProsperousHeart left a comment

Choose a reason for hiding this comment

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

For the most part the updates were ok (though one of the packages was updated oddly) ... And while I'm weary about the package file update, I have no knowledge to voice any changes. So I would have approved, but then the changes in other files are forcing a halt.

The files that appear to have been changed from linting? This is putting the file back tot he way it was before your last linting update and merge. Your new update is trying to undo that and introduce things like a spelling error that was already rectified.

So cannot approve yet. 😞

@ProsperousHeart
Copy link
Member

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

@jhoover4
Copy link
Author

jhoover4 commented Oct 4, 2023

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

Copy link
Member

@ProsperousHeart ProsperousHeart left a comment

Choose a reason for hiding this comment

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

Approved! Thanks a bunch :D

@ProsperousHeart ProsperousHeart merged commit 3a3584f into codefordallas:v1-mvp Oct 9, 2023
ProsperousHeart added a commit that referenced this pull request Oct 9, 2023
Issue58 doc4 formatting add - Since #49 has been merged and @jhoover4 already reviewed to confirm accuracy, good to go on this merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automated testing This is related to the testing files for React.js enhancement Not a requirement - enhances or improves project Not Approved - Changes Requested requirements
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

add CI option
2 participants