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

[Refactor:System] change the content of the "title check" to be more descriptive #8

Conversation

kacperswis
Copy link
Contributor

@kacperswis kacperswis commented Oct 18, 2023

change the content of the "title check" to be more helpful and descriptive

Fixes #9894

Copy link
Member

@ziesski ziesski left a comment

Choose a reason for hiding this comment

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

Addresses the issue accordingly, as well as the test.
Looks good to me, but someone with more control should take a look at the format.
@williamjallen

Copy link
Member

@williamjallen williamjallen left a comment

Choose a reason for hiding this comment

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

Changes seem reasonable to me. Thanks for this contribution @kacperswis!

@kacperswis
Copy link
Contributor Author

Changes seem reasonable to me. Thanks for this contribution @kacperswis!

The pleasure is all mine

"Dependency",
"DevDependency",
];
const allowedModules = [
Copy link
Member

Choose a reason for hiding this comment

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

Instead of repeating these two arrays across here and src/validate.ts, I would just export the arrays in src/validate.ts to reuse them here, so that it lowers the barrier of adding new values.

@bmcutler
Copy link
Member

Hmm, the title check github action should be enabled on this repository as well.
(this PRs title is descriptive, but too many characters)

@bmcutler
Copy link
Member

@kacperswis Please make the change requested by @MasterOdin
After that change and a quick re-review we can merge the PR.

@kacperswis
Copy link
Contributor Author

@kacperswis Please make the change requested by @MasterOdin After that change and a quick re-review we can merge the PR.

sure, today I will try to do this ;)

@MasterOdin MasterOdin merged commit bbe95bb into Submitty:main Nov 3, 2023
1 check passed
@MasterOdin
Copy link
Member

Thanks @kacperswis for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted PR that has been accepted during Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Title check action should fail with helpful message
5 participants