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

Tests, linting and GitHub workflow #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

aldipower
Copy link

Some more usefulness. :-)

@stuhood stuhood requested review from rileysdev and benh October 8, 2024 16:46
Copy link
Contributor

@rileysdev rileysdev left a comment

Choose a reason for hiding this comment

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

LGTM!

A few pieces of feedback about this PR (feedback which I should take myself...):

  • There's no need for the Fix deprecated loader call commit. Since this is all new code you are introducing (not changing existing code), you can squash that commit into one of the other commits.
  • If you PR is completely self explanatory from its commit messages, there is no need to add a message in your PR as a whole. Otherwise, write a useful message.


const initialize = async (context) => {};
const initialize = async (context: ExternalContext) => {};
Copy link
Contributor

Choose a reason for hiding this comment

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

context is unused.

Copy link
Author

@aldipower aldipower Oct 9, 2024

Choose a reason for hiding this comment

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

Yep, this can stay as it is and acts as a good entry point for this template, imho. :-)
"Lint fails intentionally" :-)

Copy link
Contributor

@rileysdev rileysdev Oct 9, 2024

Choose a reason for hiding this comment

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

Hmm, I'm not sure how I feel about shipping an example with unused args that fails to pass lint check. If you want to keep this as-is, I'd suggest turning off linting for this line only with a comment explaining why.

Copy link
Author

Choose a reason for hiding this comment

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

If this were an example, I would absolutely see it as you do.
As this is a template and not an example I personally have no problems with failing lint checks. Turning the linting off for this line is not a good idea I guess. If the templates gets filled with live, this switch could stay on accidentally.

@aldipower
Copy link
Author

@sp33drac3r Thanks for your general feedback! I've squashed the commits into one, looks much cleaner.

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.

2 participants