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

Feature: Add prettier GH workflow (--check only) #576

Merged

Conversation

MaoShizhong
Copy link
Contributor

@MaoShizhong MaoShizhong commented Aug 7, 2024

Because

Since we have Prettier as a dependency with a .prettierrc file and format npm script, it'd help to have a pull request workflow that checks all changed files for formatting issues (which can be fixed with a simple npm run format -- <paths to files>.

Left at a --check only workflow so any formatting that may need to be done can go in a separate commit for easier reviewing. If a --write workflow would be preferable, let me know.

If merged, I'll update the repo wiki with formatting instructions.

This PR

  • Adds a check-only Prettier GH workflow

Issue

Closes #560

Additional Information

Could not find a way to achieve this via CirclCI - could only find out how to get path filtering to allow an action to run on specific file change (and run prettier --check . on everything), rather than an action to only target changed files.

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Callbacks command: Update verbiage
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If this PR adds new features or functionality, I have added new tests
  • If applicable, I have ensured all tests related to any command files included in this PR pass, and/or all snapshots are up to date

@MaoShizhong
Copy link
Contributor Author

Hmmm, looking into why it can't detect a parser/file given it works in a test repo 🤔

@MaoShizhong
Copy link
Contributor Author

Turns out ya?ml wasn't valid in the with.files bit, so it ran due to the .yml file but didn't pass it to Prettier 🤷

@MaoShizhong
Copy link
Contributor Author

@Asartea Could you post a link here to your local repo where you managed to get it working for changed files only with CircleCI?

@Asartea
Copy link
Contributor

Asartea commented Aug 7, 2024

@Asartea Could you post a link here to your local repo where you managed to get it working for changed files only with CircleCI?

See https://github.com/Asartea/top-odin-bot-v2/tree/test-ci for the code (changes made in package.json, prettier-ci.sh, and .circleci/config.yml)

You can see some example runs (both failing and passing) at https://app.circleci.com/pipelines/github/Asartea/top-odin-bot-v2?branch=test-ci (ignore the ones where I made stupid mistakes)

@MaoShizhong
Copy link
Contributor Author

Thanks. Whoever ends up reviewing this, please let us know whether the GH workflow or CircleCI approach would be preferable. I'm personally not too familiar with CircleCI or bash scripts. If CircleCI would be preferable, then we can close this one superceded by that one.

@CouchofTomato
Copy link
Member

@KevinMulhern Can I get your input on this mate and whether CircleCI is viable.

@Asartea
Copy link
Contributor

Asartea commented Aug 7, 2024

Also, shopping for a better name for the package.json script and a better filename for the .sh file; I'm unconvinced by the current ones

Copy link
Member

@KevinMulhern KevinMulhern left a comment

Choose a reason for hiding this comment

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

LGTM 🤌

We moved to GitHub actions on the site recently and I think its the better CI platform for us. Besides the unlimited free credits on our public repos. The closer integration with Github makes it a lot simpler, which should make it easier to maintain in the long run.

@MaoShizhong MaoShizhong merged commit b14c61b into TheOdinProject:main Sep 24, 2024
3 checks passed
@MaoShizhong MaoShizhong deleted the feature/prettier-workflow branch September 24, 2024 16:39
@MaoShizhong
Copy link
Contributor Author

@KevinMulhern Do you think it's worth migrating the current circleci workflows to GH actions? From what I can see it runs (jest and eslint), I believe it shouldn't be too difficult to find the appropriate actions for them.

@Asartea
Copy link
Contributor

Asartea commented Sep 26, 2024

My two cents is that they should get migrated: its confusing to have workflows split across two different platforms

@KevinMulhern
Copy link
Member

Yeah I think it'd be worth the it.

@MaoShizhong
Copy link
Contributor Author

I'll open an issue for assignment then :)

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.

CircleCI: Add a test to run prettier
4 participants