Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add megalinter and update dependencies #204

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

grmbyrn
Copy link
Contributor

@grmbyrn grmbyrn commented Jun 4, 2024

  • Add MegaLinter
  • Update:
    • TypeScript
    • Prettier
    • Sass
    • Shiki

@grmbyrn grmbyrn requested a review from josecelano June 4, 2024 15:06
Copy link
Member

@josecelano josecelano left a comment

Choose a reason for hiding this comment

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

Hi @grmbyrn the PR looks good but there are a couple of suggestions:

Update dependencies

This could be done in an independent PR unless you need to update them for a feature included in the PR. It's OK to do it in the same PR if you are sure the PR will be merged soon and it's not complex.

Tagline

I think now it doesn't make sense to use 2 lines:

image

Maybe we can make the min width longer:

image

What do you think?

Megalinter

I think we should enable Megalinter and fix megalinter errors in the same PR otherwise we are going to have a failing workflow until we fix all errors.

You can run megalinter locally with:

npx mega-linter-runner

image

MegaLinter generates a local text report so you can fix all of them.

You should use the latest MegaLinter: https://github.com/oxsecurity/megalinter/releases/tag/v7.12.0

environment:
name: github-pages
url: ${{ steps.deployment.outputs.page_url }}

steps:
Copy link
Member

Choose a reason for hiding this comment

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

Hi @grmbyrn why did you remove the environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @josecelano github-pages was giving me this error:

Value 'github-pages' is not valid
The name of the environment used by the job.

Available expression contexts: github, inputs, vars, needs, strategy, matrix

I asked ChatGPT for advice and it recommended removing the environment as it's not necessary for deploying to GitHub Pages. It said that if you want to specify an environment, you can define it separately and reference it within your workflow.

Copy link
Member

Choose a reason for hiding this comment

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

The environment exists:

image

Sometimes I also get those warnings, but the env name could be whatever you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so do you recommend putting the environment back and ignoring the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @josecelano I've been looking for a solution to this and found an issue on GitHub Actions repo from May about including github-pages as a valid option. It's currently been placed in the backlog of projects. Do you think it's okay to put it back in and ignore the error?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @josecelano I've been looking for a solution to this and found an issue on GitHub Actions repo from May about including github-pages as a valid option. It's currently been placed in the backlog of projects. Do you think it's okay to put it back in and ignore the error?

Yes, @graeme, I would put it back, and I would open a new issue to inform other contributors they could have that error until that schema is updated. We can close it when it is updated, since I think we can't do anything else.

@grmbyrn
Copy link
Contributor Author

grmbyrn commented Jun 5, 2024

Hi @grmbyrn the PR looks good but there are a couple of suggestions:

Update dependencies

This could be done in an independent PR unless you need to update them for a feature included in the PR. It's OK to do it in the same PR if you are sure the PR will be merged soon and it's not complex.

Hi @josecelano okay, I'll remember to dependencies separately unless necessary.

Tagline

I think now it doesn't make sense to use 2 lines:

image

Maybe we can make the min width longer:

image

What do you think?

Yes, I agree that this looks better on one line and can fix that.

Megalinter

I think we should enable Megalinter and fix megalinter errors in the same PR otherwise we are going to have a failing workflow until we fix all errors.

You can run megalinter locally with:

npx mega-linter-runner

image

MegaLinter generates a local text report so you can fix all of them.

You should use the latest MegaLinter: https://github.com/oxsecurity/megalinter/releases/tag/v7.12.0

Okay, should I fix megalinter errors in this PR?

@josecelano
Copy link
Member

Hi @grmbyrn the PR looks good but there are a couple of suggestions:

Update dependencies

This could be done in an independent PR unless you need to update them for a feature included in the PR. It's OK to do it in the same PR if you are sure the PR will be merged soon and it's not complex.

Hi @josecelano okay, I'll remember to dependencies separately unless necessary.

Tagline

I think now it doesn't make sense to use 2 lines:
image
Maybe we can make the min width longer:
image
What do you think?

Yes, I agree that this looks better on one line and can fix that.

Megalinter

I think we should enable Megalinter and fix megalinter errors in the same PR otherwise we are going to have a failing workflow until we fix all errors.
You can run megalinter locally with:

npx mega-linter-runner

image
MegaLinter generates a local text report so you can fix all of them.
You should use the latest MegaLinter: https://github.com/oxsecurity/megalinter/releases/tag/v7.12.0

Okay, should I fix megalinter errors in this PR?

Hi @grmbyrn yes, if you like. But I will probably take you long. It's usually hard to fix all the MegaLinter errors if it's not installed from the first commit. I would open a new PR just for that.

@grmbyrn
Copy link
Contributor Author

grmbyrn commented Jun 5, 2024

Hi @grmbyrn the PR looks good but there are a couple of suggestions:

Update dependencies

This could be done in an independent PR unless you need to update them for a feature included in the PR. It's OK to do it in the same PR if you are sure the PR will be merged soon and it's not complex.

Hi @josecelano okay, I'll remember to dependencies separately unless necessary.

Tagline

I think now it doesn't make sense to use 2 lines:
image
Maybe we can make the min width longer:
image
What do you think?

Yes, I agree that this looks better on one line and can fix that.

Megalinter

I think we should enable Megalinter and fix megalinter errors in the same PR otherwise we are going to have a failing workflow until we fix all errors.
You can run megalinter locally with:

npx mega-linter-runner

image
MegaLinter generates a local text report so you can fix all of them.
You should use the latest MegaLinter: https://github.com/oxsecurity/megalinter/releases/tag/v7.12.0

Okay, should I fix megalinter errors in this PR?

Hi @grmbyrn yes, if you like. But I will probably take you long. It's usually hard to fix all the MegaLinter errors if it's not installed from the first commit. I would open a new PR just for that.

Okay, I'll open a new PR for the MegaLinter errors. Is there anything else to fix before merging this PR? The environment in deploy.yml is still giving me the error I mentioned.

Add back in removed environment. Error being shown raised in GitHub Actions with this issue: actions/deploy-pages#344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants