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

Reduce config migration whitespace changes for arrays #16383

Closed
Tracked by #16359
rarkins opened this issue Jul 2, 2022 · 11 comments
Closed
Tracked by #16359

Reduce config migration whitespace changes for arrays #16383

rarkins opened this issue Jul 2, 2022 · 11 comments
Assignees
Labels
core:config Related to config capabilities and presets priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)

Comments

@rarkins
Copy link
Collaborator

rarkins commented Jul 2, 2022

What would you like Renovate to be able to do?

Migrate a config containing arrays without changing the white space / line returns unnecessarily. For example here:

image

The first change is 100% unnecessary and should be avoided. The second would be a "nice to have" if we can avoid the change.

If you have any ideas on how this should be implemented, please tell us here.

We could add a new function which attempts to "harmonize" the result with the original, focusing on arrays of strings first.

Parse and iterate through the final config, looking for arrays of strings. If the same array with same contents exists in the original, then replace the raw content for that array in the migrated config with the raw content of the original config. Of course we must make sure that the result parses and is exactly the same.

Is this a feature you are interested in implementing yourself?

No

@rarkins rarkins added type:feature Feature (new functionality) priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:ready core:config Related to config capabilities and presets labels Jul 2, 2022
@viceice
Copy link
Member

viceice commented Jul 2, 2022

what about using prettier API to format when a prettier config is found. 🤔 the API is very simple to use.

@rarkins
Copy link
Collaborator Author

rarkins commented Jul 2, 2022

It's a possibility. Won't cover all use cases though. BTW prettier still passes/accepts these changes AFAICT

@viceice
Copy link
Member

viceice commented Jul 2, 2022

on my vscode it will revert the array changes on format. 🤔

@rarkins
Copy link
Collaborator Author

rarkins commented Jul 2, 2022

I assumed that because our CI is not failing. Does prettier accept them but prettier --fix change them?

@viceice
Copy link
Member

viceice commented Jul 2, 2022

maybe, not validated. most of the time the pre-commit hill will fix that for almost all my repos

@Gabriel-Ladzaretti
Copy link
Collaborator

Gabriel-Ladzaretti commented Jul 19, 2022

Here is a POC for recursively restore user format when key value pairs(objects) or keys(arrays) are the same between the original config file and the migrated to be committed string.
this supports json5 (trailing commas, quotes etc.).

some future work can be done for doing the same for migrated keys.
as i was not able to come up with an easy way to do the conversion via the MigrationService class, this can be done in future feature request like adding a key conversion map. once this future request is implemented, the change to the current PR is basically just getting the converted key.

An edge case that is not handled is a value type change (e.g. baseBranches), if this is common i can address it as well.

left: json config migration
middle: before this PR config migrateion
right: json5 config migration

image

if/once this approach is accepted ill had the needed tests and open a PR.

@viceice
Copy link
Member

viceice commented Jul 19, 2022

is this still an issue since we ran prettier? I don't like the manual way. Looks pretty hacky on JSON manipulation

@viceice
Copy link
Member

viceice commented Jul 19, 2022

@rarkins Isn't this fixed by:

@rarkins
Copy link
Collaborator Author

rarkins commented Jul 19, 2022

I expected this would be fixed "well enough". Not sure if the remaining edge cases are worth the complexity

@Gabriel-Ladzaretti
Copy link
Collaborator

Gabriel-Ladzaretti commented Jul 19, 2022

heres a "before" with a .prettierrc, seems nice 😅.
i guess i brushed up my recursion skills, which is nice.
should we close this one?

@rarkins rarkins closed this as completed Jul 19, 2022
@nabeelsaabna
Copy link
Contributor

what if we update the renovate.json with prettier regardless of the migration, then once migration is required the diff will be minimal.
Too much ? 🤔

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core:config Related to config capabilities and presets priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)
Projects
None yet
Development

No branches or pull requests

4 participants