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

Make GitHub token optional #268

Merged
merged 5 commits into from
Oct 28, 2024
Merged

Conversation

mandrasch
Copy link
Collaborator

The Issue

#266

How This PR Solves The Issue

  • add check in the beginning of api.ts, IIFE (Immediately Invoked Function Expression) pattern
  • add checks to all functions calling github API, return empty arrays

Manual Testing Instructions

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

I confirm that it works without GitHub token.

@mandrasch, please also update this in two places (for the home and about page):

-parseInt(repoData.stargazers_count)
+parseInt(repoData.stargazers_count || 0)

because it showed NaN.

And please run ddev npm run textlint:fix on the repo.

I thought it would change the spaces around if(){} (I see you prefer it without spaces, but other code has them), please make it the same.

@mandrasch
Copy link
Collaborator Author

mandrasch commented Oct 25, 2024

Thx for feedback! Re: spaces / formatting

Oh sorry, haven't had prettier autoFormatOnSave enabled in VSCode. This can be added in .vscode/settings.json as well (and checks are also possible for PRs via prettier CLI, guess you know that already):

    "editor.defaultFormatter": "esbenp.prettier-vscode"
    "editor.formatOnSave": true,

Just formatted with prettier, but lot's of other changes in places I did not touch. Seems like https://prettier.io/docs/en/options.html#semicolons is not set correctly / files were not formatted before with prettier.

image

Need to re-check in the following days.

@mandrasch
Copy link
Collaborator Author

@stasadev api.ts was not formatted with prettier before. Should I re-format the whole file in this PR? Or just do my changes first?

@stasadev
Copy link
Member

I see, given that we don't have formatting here, it would be nice to have one 🙂

Since there aren't too many changes in this PR, I don't mind if you add formatting for that file here, thanks!

@mandrasch
Copy link
Collaborator Author

mandrasch commented Oct 25, 2024

👍 I re-formatted api.ts manually

image image

I also added .ddev-directory to .prettierignore to run my checks (see below)


Optional information

I also wanted to prepare .vscode/settings.json to enable auto-formatting in a fine-tuned way for js & ts. But I would leave it disabled for javascript for ts and js, since there are some unformatted files in the repo. Did not want to disturb / confuse future contributors.

But then I noticed that .vscode/ is gitignored for this repo. 🤔

{
  "editor.defaultFormatter": "esbenp.prettier-vscode",
  "editor.formatOnSave": false,
  "[typescript]": {
    "editor.formatOnSave": false
  },
  "[javascript]": {
    "editor.formatOnSave": false
  }
}

I checked status of formatting with ddev exec 'npx prettier . --check':

Checking formatting...
[warn] .prettierrc
[warn] astro.config.mjs
[warn] README.md
[warn] src/const.ts
[warn] src/content/authors/bernardo-martinez.md
[warn] src/content/authors/jochen-roth.md
[warn] src/content/authors/lasse-blomenkemper.md
[warn] src/content/authors/matt-stein.md
[warn] src/content/authors/tony-groff.md
[warn] src/content/blog/2023-review.md
[warn] src/content/blog/2024-plans.md
[warn] src/content/blog/501c3.md
[warn] src/content/blog/advanced-add-on-contributor-training.md
[warn] src/content/blog/amd64-on-apple-silicon-ddev.md
[warn] src/content/blog/anatomy-advanced-ddev-addon.md
[warn] src/content/blog/contributor-training.md
[warn] src/content/blog/ddev-and-docker-healthchecks-technote.md
[warn] src/content/blog/ddev-and-xz-backdoor.md
[warn] src/content/blog/ddev-backups.md
[warn] src/content/blog/ddev-debug-test-contributor-training.md
[warn] src/content/blog/ddev-diffy-introduction.md
[warn] src/content/blog/ddev-docker-architecture.md
[warn] src/content/blog/ddev-docker-image-maintenance.md
[warn] src/content/blog/ddev-in-gitlab-ci.md
[warn] src/content/blog/ddev-local-automated-testing.md
[warn] src/content/blog/ddev-name-resolution-wildcards.md
[warn] src/content/blog/ddev-oct-2024-newsletter.md
[warn] src/content/blog/ddev-website-for-contributors.md
[warn] src/content/blog/docker-performance-2023.md
[warn] src/content/blog/docker-providers.md
[warn] src/content/blog/drupal7-drupal9-migration-ddev-acquia-migrate-accelerate.md
[warn] src/content/blog/drupalcon-barcelona-2024.md
[warn] src/content/blog/drupalcon-pittsburgh.md
[warn] src/content/blog/drupalcon-portland-wrapup.md
[warn] src/content/blog/expanding-ddev-maintainer-team.md
[warn] src/content/blog/extending-ddev-commands-with-scripts.md
[warn] src/content/blog/golang-debugging.md
[warn] src/content/blog/how-to-give-and-get-community-support.md
[warn] src/content/blog/introducing-maintainer-stas.md
[warn] src/content/blog/lets-fund-stas-maintainer.md
[warn] src/content/blog/maintaining-ddev-tests-contributor-training.md
[warn] src/content/blog/mariadb-dump-breaking-change.md
[warn] src/content/blog/oh-my-zsh-using-custom-commands-and-other-goodies-to-add-to-ddev.md
[warn] src/content/blog/randy-in-patagonia.md
[warn] src/content/blog/recruiting-maintainers.md
[warn] src/content/blog/release-v1.23.5-auto-port-assignment.md
[warn] src/content/blog/setting-up-a-go-development-environment.md
[warn] src/content/blog/traefik-configuration-contributor-training.md
[warn] src/content/blog/unleashing-php-development-with-ddev.md
[warn] src/content/blog/working-with-vite-in-ddev.md
[warn] src/content/blog/xdebug-debugging.md
[warn] src/content/config.ts
[warn] src/pages/blog/feed.json.js
[warn] src/pages/resources/featured-sponsors-darkmode.svg.js
[warn] src/pages/resources/featured-sponsors.svg.js
[warn] src/styles/global.css
[warn] tailwind.config.cjs
[warn] tsconfig.json
[warn] Code style issues found in 58 files. Run Prettier with --write to fix.

This check could be added to .github/workflows/test.yml as well.

Guess we should move this discussion to another PR (if relevant)? 🤔


Copy link
Member

@stasadev stasadev 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!

Yes, formatting for the rest of the code, and linter check in GitHub Actions can be done later in another PR after this goes in.

@rfay rfay requested a review from bmartinez287 October 25, 2024 18:17
@rfay
Copy link
Member

rfay commented Oct 25, 2024

@bmartinez287 please see if you like it.

@rfay rfay marked this pull request as ready for review October 25, 2024 18:18
@bmartinez287
Copy link
Collaborator

bmartinez287 commented Oct 27, 2024

looks good, thanks for the PR.
Adding the note to the README is a great improvement.

@rfay rfay merged commit 36e9abc into ddev:main Oct 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants