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 formatting as workflow (as requested) #13982

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tobezdev
Copy link
Contributor

@tobezdev tobezdev commented May 15, 2024

This PR is not a domain creation request.
Reference pull #13966

Version 2 of adding Prettier formatting to is-a-dev/register!
New in this version:

  • moved manual workflow to automatic:
    • [+] .github/workflows/formatting.yml - create the workflow
    • {~] package.json - added configuration for the formatter
    • [~] package-lock.json - added script to run formatting on-change
    • [+] .prettierrc - add configuration for formatting to run with
    • [+] .prettierignore - add a list of files for formatting to ignore

Please request changes on .prettierignore before merging this to ensure the formatter only edits the correct files. You may also want to check/test the workflow to ensure it runs as intended.

Copy link
Member

@MaskDuck MaskDuck left a comment

Choose a reason for hiding this comment

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

thank you for your second attempt at this, I highly value that!

Comment on lines 11 to 18
format:
if: github.repository == 'is-a-dev/register'
runs-on: ubuntu-latest
permissions: [read-all, write-all]
steps:
run: npx prettier domains/* --check
if github.working-directory == 'domains/':
run: npx prettier domains/* --write
Copy link
Member

Choose a reason for hiding this comment

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

I've got something to say about this:

  1. push the changes. an approach to this would be to use actions/checkout.
  2. I don't think we need write-all, only write: contents will be enough.

Copy link
Member

Choose a reason for hiding this comment

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

@orxngc orxngc added the enhancement New feature or request label May 15, 2024
.github/workflows/formatting.yml Outdated Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I don’t believe we need a package-lock.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You already had a package-lock.json file, I just updated it to include the prettier module.

Copy link
Member

Choose a reason for hiding this comment

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

There isn't one on the main branch at the moment, unless my client is bugging.

Copy link
Contributor Author

@tobezdev tobezdev May 16, 2024

Choose a reason for hiding this comment

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

I see one directly on https://github.com/is-a-dev/register.
I also forked the repo directly from Github, without adding the rest of the package-lock.json.
Not sure what's going on here, I'll take a proper look later when I'm at my computer.

I'm also not sure why the diff for this pr shows me as adding the entire package-lock.json - I'll also have a look into this.

Copy link
Member

@wdhdev wdhdev May 17, 2024

Choose a reason for hiding this comment

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

.github/workflows/formatting.yml Outdated Show resolved Hide resolved
.github/workflows/formatting.yml Outdated Show resolved Hide resolved
tobezdev added a commit to tobezdev/register-formatted-workflows that referenced this pull request May 18, 2024
tobezdev added a commit to tobezdev/register-formatted-workflows that referenced this pull request May 18, 2024
Copy link
Member

@wdhdev wdhdev left a comment

Choose a reason for hiding this comment

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

Can you ensure this works by running it in a test repo?

@tobezdev
Copy link
Contributor Author

Sure @wdhdev, I'll do it later on today :)

repository: '${{ github.repository }}'
token: '${{ github.token }}'
branches:
- main
Copy link
Member

Choose a reason for hiding this comment

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

this won't push, check out this part

@tobezdev tobezdev requested review from wdhdev and MaskDuck May 22, 2024 21:15
@wdhdev
Copy link
Member

wdhdev commented May 25, 2024

Sure @wdhdev, I'll do it later on today :)

Have you done this yet? If so, please send me the action run.

@omsenjalia
Copy link
Contributor

Sure @wdhdev, I'll do it later on today :)

Any updates?

Copy link

github-actions bot commented Jun 4, 2024

This pull request has been marked as stale due to inactivity and will be closed. Comment anything on this PR to prevent it

@github-actions github-actions bot added the stale label Jun 4, 2024
@CuteDog5695
Copy link
Member

@tobezdev Any updates?

@tobezdev
Copy link
Contributor Author

tobezdev commented Jun 5, 2024

Yes. I have managed to recreate the workflow on a private test repo. The formatting seemed to work fine, but I'm trying to troubleshoot a check failure (though it seems to work fine)
I'll add another comment here when I find a fix to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants