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 import-ordering editorconifg generation #1011

Merged
merged 3 commits into from
Dec 16, 2020

Conversation

romtsn
Copy link
Collaborator

@romtsn romtsn commented Dec 14, 2020

@Tapchicoma not sure why you converted the properties to List<PatternEntry> but writing to editorconfig didn't work properly, because:

  • PatternEntry's toString method was messed up
  • By default List<PatternEntry>.toString() returns something like [*,kotlin.**,java.**,^] - mind the braces. This is an incorrect value if we wanna write to the standard ij_kotlin_imports_layout, to properly generate it we would need to use joinToString but we don't know the type in the getEditorConfigValue..

So the easiest path I found is to revert properties to be strings, but not sure if it's the best approach. Maybe we could introduce some interface like EditorconfigWriteable which has a toString method and put it as an upper bound for generic EditorConfigProperty<T : EditorconfigWriteable>

@romtsn romtsn requested a review from Tapchicoma December 14, 2020 22:11
@Tapchicoma
Copy link
Collaborator

@romtsn this experimental api is in alpha state so it is fine to find such issues and fix them 🙂 Thank you for finding this one 👏

Regarding the issue itself - current code should correctly parse *,kotlin.**,java.**,^ values (without [ and ]), check ImportLayoutParserTest. Ec4j library does not modify property value and passes it as-is into provided parser.

The real "issue" is in .editorconfig generator - it uses default T.toString() implementation to write back this fields. In this particular case it is AbstractCollection.toSting() one.

My vision how it could be fixed:

  • EditorConfigProperty will get new field propertyWriter that by default will use toString():
    public data class EditorConfigProperty<T>(
        public val type: PropertyType<T>,
        public val defaultValue: T,
        public val defaultAndroidValue: T = defaultValue,
        public val propertyWriter: (T) -> String = { it.toString() }
    )
  • UsesEditorConfigProperties interface will get new method:
    public fun <T> EditorConfigProperties.writeEditorConfigProperty(
        property: EditorConfigProperty<T>,
        isAndroidCodeStyle: Boolean
    ): String {
        return property.propertyWriter(getEditorConfigValue(property, isAndroidCodeStyle))
    }
  • EditorConfigGenerator will use writeEditorConfigProperty new method instead of .toString()

Then to fix ImportOrderingRule properties should provide non-default propertyWriter, something like it.joinToString()

@Tapchicoma
Copy link
Collaborator

not sure why you converted the properties to List<PatternEntry>

imho it makes rule code easier as you not deal with string of patterns, but already parsed list of pattern that it is easier to work with

@romtsn
Copy link
Collaborator Author

romtsn commented Dec 15, 2020

imho it makes rule code easier as you not deal with string of patterns, but already parsed list of pattern that it is easier to work with

Yeah, makes sense 👍

Regarding the issue itself - current code should correctly parse *,kotlin.,java.,^ values (without [ and ]), check ImportLayoutParserTest. Ec4j library does not modify property value and passes it as-is into provided parser.

That's true, it will correctly parse it. But when you try to write it back using PatternEntry.toString() method, it will not be the same (but that's because I did not implement the toString method correctly initially - already fixed)

As for your proposal, it looks good I will follow up on this soon 👍

@romtsn
Copy link
Collaborator Author

romtsn commented Dec 16, 2020

@Tapchicoma updated accordingly 👍

Copy link
Collaborator

@Tapchicoma Tapchicoma left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

2 participants