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 CI Rule for Testing camelCase Naming Convention #10848

Open
jNullj opened this issue Jan 24, 2025 · 2 comments
Open

Add CI Rule for Testing camelCase Naming Convention #10848

jNullj opened this issue Jan 24, 2025 · 2 comments
Labels
developer-experience Dev tooling, test framework, and CI

Comments

@jNullj
Copy link
Member

jNullj commented Jan 24, 2025

Develop a CI rule to verify that all identifiers in the codebase that are not dictated by upstream adhere to the camelCase naming convention.

@jNullj jNullj added the developer-experience Dev tooling, test framework, and CI label Jan 24, 2025
@chris48s
Copy link
Member

As a bit of background, the reason we've ended up ignoring this is because of

shields/eslint.config.js

Lines 42 to 50 in 2c32e02

camelcase: [
'error',
{
ignoreDestructuring: true,
properties: 'never',
ignoreGlobals: true,
allow: ['^UNSAFE_'],
},
],

History/context: #6319

Fundamentally, its hard to consistently enforce camelCase everywhere when we deal with lots of API responses that contain snake_case or kebab-case names. Simultaneously, this allowed some of our own identifiers to fall through the cracks.

I think doing this as a static analysis check is going to be really difficult, but what we probably can do is make it a check at service registration time. So maybe somewhere like:

route: Joi.alternatives().try(
Joi.object({
pattern: Joi.string().required(),
queryParams: arrayOfStrings,
}),
Joi.object({
format: Joi.string().required(),
queryParams: arrayOfStrings,
}),

async function loadServiceClasses(servicePaths) {
if (!servicePaths) {
servicePaths = getServicePaths('*.service.js')
}
const serviceClasses = []
for await (const servicePath of servicePaths) {
const currentServiceClasses = Object.values(
await import(`file://${servicePath}`),
).flatMap(element =>
typeof element === 'object' ? Object.values(element) : element,
)
if (currentServiceClasses.length === 0) {
throw new InvalidService(
`Expected ${servicePath} to export a service or a collection of services`,
)
}
currentServiceClasses.forEach(serviceClass => {
if (serviceClass && serviceClass.prototype instanceof BaseService) {
// Decorate each service class with the directory that contains it.
serviceClass.serviceFamily = servicePath
.replace(serviceDir, '')
.split(path.sep)[1]
serviceClass.validateDefinition()
return serviceClasses.push(serviceClass)
}
throw new InvalidService(
`Expected ${servicePath} to export a service or a collection of services; one of them was ${serviceClass}`,
)
})
}
assertNamesUnique(
serviceClasses.map(({ name }) => name),
{
message: 'Duplicate service names found',
},
)
const routeSummaries = []
serviceClasses.forEach(function (serviceClass) {
if (serviceClass.openApi) {
for (const route of Object.values(serviceClass.openApi)) {
routeSummaries.push(route.get.summary)
}
}
})
assertNamesUnique(routeSummaries, {
message: 'Duplicate route summary found',
})
return serviceClasses
}

There we can specifically inspect the queryParams schema for a service in a context where we know what that is/means.

Of course, the next issue is: At the moment we can't just say "all keys in the query params schema must be camelCase" because we have lots that aren't, so we need a way to incrementally adopt it, or adopt it at the same point we make changes under discussion in #10804 (e.g: only throw InvalidService() if there's a snake_case variant of a param and a camelCase variant is not also accepted) etc.

@jNullj
Copy link
Member Author

jNullj commented Jan 25, 2025

I had in mind a npm task that users can run to return what services dont use camelCase, then implement a github action that checks and only warns if relevant users don't use camelCase.
I was thinking using groups of regex but that would be hard and not very efficient use of time, i like your idea with runtime tests more, i think its better to use it at a separate task and not at normal server load.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

No branches or pull requests

2 participants