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

internal: add type to mutateData #27932

Merged
merged 4 commits into from
Nov 19, 2024
Merged

Conversation

mshima
Copy link
Member

@mshima mshima commented Nov 19, 2024


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@mshima mshima marked this pull request as draft November 19, 2024 19:59
@mshima mshima marked this pull request as ready for review November 19, 2024 20:20
@@ -202,7 +202,7 @@ export default class AngularGenerator extends BaseApplicationGenerator {

return returnValue;
},
});
} as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't it as F?

Copy link
Contributor

Choose a reason for hiding this comment

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

if it's legit, any other comment of this PR should be ignored

@@ -152,7 +153,7 @@ export const loadDerivedAppConfig = ({ application }: { application: any }) => {

projectDescription: ({ projectDescription, baseName }) => projectDescription ?? `Description for ${baseName}`,
endpointPrefix: ({ applicationType, lowercaseBaseName }) => (applicationType === 'microservice' ? `services/${lowercaseBaseName}` : ''),
});
} as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

as ApplicationType?

@@ -285,7 +289,7 @@ export function derivedPrimaryKeyProperties(primaryKey) {
typeLong: primaryKey.type === LONG,
typeInteger: primaryKey.type === INTEGER,
typeNumeric: !primaryKey.composite && primaryKey.fields[0].fieldTypeNumeric,
});
} as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

as PK?

microserviceName: ({ builtIn }) => (!builtIn && config.applicationType === MICROSERVICE ? config.baseName : undefined),
});
if (entity.searchEngine === true && (!entity.microserviceName || entity.microserviceName === config.baseName)) {
} as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

as E?

@@ -647,7 +652,7 @@ function preparePostEntityCommonDerivedPropertiesNotTyped(entity: any) {
entity.eagerLoad ||
// Fetch relationships if otherEntityField differs otherwise the id is enough
(ownerSide && otherEntity.primaryKey.name !== otherEntityField)),
});
} as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

as E

}

mutateData(relationship, {
__override__: false,
columnDataType: data => data.otherEntity.columnType,
columnRequired: data => data.nullable === false || data.relationshipRequired,
liquibaseGenerateFakeData: data => data.columnRequired && data.persistableRelationship && !data.collection,
});
} as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

Relationship

@@ -161,7 +159,7 @@ export const loadDerivedServerConfig = ({ application }: { application: any }) =
authenticationUsesCsrf: ({ authenticationType }) => [OAUTH2, SESSION].includes(authenticationType),
imperativeOrReactive: ({ reactive }) => (reactive ? 'reactive' : 'imperative'),
generateSpringAuditor: ctx => ctx.databaseTypeSql || ctx.databaseTypeMongodb || ctx.databaseTypeNeo4j || ctx.databaseTypeCouchbase,
});
} as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

as ApplicationType

@@ -33,7 +34,7 @@ const { MapperTypes } = entityOptions;
const { MAPSTRUCT } = MapperTypes;
const { INTEGER, LONG, UUID } = CommonDBTypes;

export default function prepareField(entityWithConfig, field, generator) {
export default function prepareField(entityWithConfig, field: Field & any, generator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Introduce ServerField type?

@@ -52,7 +53,7 @@ export default function prepareField(entityWithConfig, field, generator) {
fieldJavadoc: formatDocAsJavaDoc(field.documentation, 4),
fieldApiDescription: formatDocAsApiDescription(field.documentation),
propertyApiDescription: ({ fieldApiDescription }) => fieldApiDescription,
});
} as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

ServerField?

import { mutateData } from '../../../lib/utils/object.js';
import { formatDocAsApiDescription, formatDocAsJavaDoc } from '../../java/support/doc.js';

export function prepareRelationship({ relationship }: { relationship: any; entity: any }) {
export function prepareRelationship({ relationship }: { relationship: Relationship; entity: any }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ServerEntity?

@DanielFran DanielFran merged commit 5135b2f into jhipster:main Nov 19, 2024
63 checks passed
@DanielFran
Copy link
Member

@Tcharl sorry, did not see your reviews before I committed

@mshima
Copy link
Member Author

mshima commented Nov 19, 2024

@Tcharl mutateData now expects a Record as context but is receiving any objects, the others parameters expects a type based on context type.
Those data doesn't match current application/entity/field/relationship types and are too many changes to handle at once.
That's the reason that as any are required.

@mshima mshima deleted the more-types-2 branch November 19, 2024 20:46
@Tcharl
Copy link
Contributor

Tcharl commented Nov 20, 2024

@DanielFran @mshima No problem! I even think I've made the review post merge.
I'm trying to understand the best practice on the core gen to contribute properly and inline with the conventions.
I'm also sorry to bother you with stupid questions, typescript is not my native programming language and these ones help me to provide better contribution. Clean core is a something that is important and I want to help as much as I can

@mraible mraible added this to the 8.8.0 milestone Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants