CI (Continuous Integration) pipeline added #79
Conversation
| - uses: actions/checkout@v2 | ||
| with: | ||
| fetch-depth: 0 # Fetch all history for all branches and tags | ||
| - name: Use Node.js |
| @@ -34,3 +34,14 @@ yarn-error.log* | |||
| # typescript | |||
| *.tsbuildinfo | |||
| next-env.d.ts | |||
|
|
|||
| # Ignore all files in the app directory | |||
There was a problem hiding this comment.
Why we are adding these here?
| /app/* | ||
| /components/* | ||
|
|
||
| # Except the about/page.tsx file |
There was a problem hiding this comment.
Why we are adding these here?
There was a problem hiding this comment.
When I commit the file the prettier and eslinter fixes each and every file and because I did't wanted to commit .
There was a problem hiding this comment.
These files are the files on which check should happen. Once Linter, and format will execute yes they will show errors and requires fixes across project. This is wrong to put the files in gitignore.
| "autoprefixer": "^10", | ||
| "eslint": "^8", | ||
| "eslint-config-next": "14.0.0", | ||
| "eslint": "^8.57.0", |
There was a problem hiding this comment.
These should be dev-dependencies?
.eslintrc.cjs
Outdated
There was a problem hiding this comment.
ESLint configuration files often use CommonJS syntax, by using .cjs , you explicitly tell node.js and eslint that this configuration file uses commonJS syntax, avoiding potential conflicts or errors.
There was a problem hiding this comment.
have you tried only .eslintrc ?
| steps: | ||
| - uses: actions/checkout@v2 | ||
| with: | ||
| fetch-depth: 0 # Fetch all history for all branches and tags |
There was a problem hiding this comment.
Do we need to all branches? Also, we don't have any tags
There was a problem hiding this comment.
should I add this
fetch-depth: 1 # Fetch only the latest commit
There was a problem hiding this comment.
I think we don't need it. Please check
| - name: Install dependencies | ||
| run: npm install | ||
|
|
||
| - name: Run lint |
There was a problem hiding this comment.
which lint we are running. Can we please add the name
There was a problem hiding this comment.
Should i rename it to
- name: Run ESLint
| - name: Run lint | ||
| run: npm run lint | ||
|
|
||
| - name: Run format check |
There was a problem hiding this comment.
which format we are running. Can we please add the name
There was a problem hiding this comment.
Run Prettier to check code formatting
- name: Run Prettier check
There was a problem hiding this comment.
these changes should not be here
There was a problem hiding this comment.
They were done by the prettier formatter , shuld I drop them in the next commit?
There was a problem hiding this comment.
Have you run the PR at your local? These changes should be for across the project
types/png.d.ts
Outdated
There was a problem hiding this comment.
declaring modules for file types like *.png, *.svg, *.jpeg, and *.jpg in a TypeScript its was a eslinter error
|
@Neha |
The whole purpose of having Eslint, and Prettier in CI is to get such errors and fix them. Putting the files in gitignore to ignore these errors will not serve any purpose. |
There was a problem hiding this comment.
Suggestion: I would generally suggest using the new stable versions of actions and always use pinned GHA (here's why -- tl;dr; for better security and to prevent us from SLSA attacks)
| with: | ||
| fetch-depth: 0 # Fetch all history for all branches and tags | ||
| - name: Use Node.js | ||
| uses: actions/setup-node@v2 |
There was a problem hiding this comment.
Same goes for this:
| uses: actions/setup-node@v2 | |
| uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 #v4.0.2 |
| @@ -148,4 +148,4 @@ export default async function page() { | |||
| </section> | |||
| </> | |||
| ); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Suggestion (non-blocking): Not sure, why our formatter removes the newline at the end, usually it's a good practice to have newline at the end.
There was a problem hiding this comment.
My guess is formatter is not installed properly.
Co-authored-by: Siddhant Khare <[email protected]>
|
Hey @akshat99812 would you be able to work on the code comments? |
|
@Neha Sure, I'd be happy to help! Could you please clarify what specific tasks you want me to work on regarding the code comments? For example, are you looking for me to add comments to existing code, review and improve current comments, or something else? |
I didn't got your question. |
Hey @akshat99812, we are just suggesting to address the PR review comments that Neha & I left above |
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
This pull request introduces a CI (Continuous Integration) pipeline to our project. The CI pipeline automates the process of testing and building our codebase, ensuring that code changes are validated and merged seamlessly. This enhancement addresses issue #3 and aims to improve our development workflow by catching potential issues early.
Fixes #3
Snapshots/Videos:
Type of change
Added tests?
Internationalization Support?
Steps to test the feature:
Snapshot of the test-cases that are passing:
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)