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

Fix validation of modified declarations #1066

Merged
merged 14 commits into from
Apr 8, 2024

Conversation

Ndpnt
Copy link
Member

@Ndpnt Ndpnt commented Mar 29, 2024

Rewrite the method used to identify modified services term types. It address two key issues:

  • Fixed results when a new service is added: Previously, the method failed to detect newly added services, leading to CI tests consistently validating pull requests that introduced new services.
  • Fixed results when filters are modified: Previously, the method failed to detect services related to updated filters, leading to CI tests consistently validating pull requests that only modified services filters.

Others improvements:

Improve the method for identifying modified service term types by addressing issues with new service additions and modified filters.
@Ndpnt Ndpnt force-pushed the fix-modified-declarations-validations branch from 9e86530 to ac0196a Compare March 29, 2024 14:13
@Ndpnt Ndpnt enabled auto-merge April 8, 2024 08:12
@Ndpnt Ndpnt disabled auto-merge April 8, 2024 08:13
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Congrats for the improvements, the fixes, the tests, and dropping a dependency! 👏

I find the code unusually hard to understand, with very long functions, shadowing variables and inconsistent data structures.

"select": "body"
},
"Privacy Policy": {
"fetch": "https://domain.example/tos",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"fetch": "https://domain.example/tos",
"fetch": "https://domain.example/privacy",


return JSON.parse(fileContent);
} catch (error) {
return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return undefined;
return;

scripts/declarations/utils/index.js Outdated Show resolved Hide resolved
const servicesTermsTypes = {};

await Promise.all(modifiedFilePaths.map(async modifiedFilePath => {
const serviceId = DeclarationUtils.filePathToServiceId(modifiedFilePath);
const serviceId = DeclarationUtils.getServiceIdFromFilePath(modifiedFilePath);
Copy link
Member

Choose a reason for hiding this comment

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

Extract the block in map to new function, it is too long to be declared this way in an iteration 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not understand what you mean here

Copy link
Member

Choose a reason for hiding this comment

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

The whole anonymous function that is defined and called as the map parameter is huge. I believe it would help with readability if it was extracted into its own named function.

Copy link
Member Author

Choose a reason for hiding this comment

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

With the code simplification, I no longer find it useful.

const declaration = await this.getJSONFile(`declarations/${serviceId}.json`, this.defaultBranch);
// TODO: Implement AST comparison between original and modified "filters" files to detect changes in functions, enabling identification of terms whose types depend on modified functions.
// Due to the complexity involved, temporarily returning all term types.
servicesTermsTypes[serviceId] = Object.keys(declaration.documents);
Copy link
Member

Choose a reason for hiding this comment

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

Since we map, why use an outside variable instead of using the return value of map?

scripts/declarations/utils/index.test.js Outdated Show resolved Hide resolved
scripts/declarations/utils/index.test.js Outdated Show resolved Hide resolved
scripts/declarations/utils/index.test.js Outdated Show resolved Hide resolved
scripts/declarations/utils/index.test.js Outdated Show resolved Hide resolved
scripts/declarations/utils/index.test.js Outdated Show resolved Hide resolved
Ndpnt and others added 8 commits April 8, 2024 10:58
Co-authored-by: Matti Schneider <matti@opentermsarchive.org>
Co-authored-by: Matti Schneider <matti@opentermsarchive.org>
@Ndpnt Ndpnt merged commit a544182 into main Apr 8, 2024
9 checks passed
@Ndpnt Ndpnt deleted the fix-modified-declarations-validations branch April 8, 2024 12:29
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.

None yet

3 participants