karakum-team / karakum

Converter of TypeScript declaration files to Kotlin declarations
Apache License 2.0
37 stars 0 forks source link

Windows-style input path generates weird super-long package names #7

Closed joffrey-bion closed 1 year ago

joffrey-bion commented 1 year ago

Example input config:

{
  "input" : "C:\\Projects\\kotlin-blueprintjs\\build\\js\\node_modules\\@blueprintjs\\icons\\lib\\esm\\index.d.ts",
  "output" : "C:\\Projects\\kotlin-blueprintjs\\kotlin-blueprintjs-icons\\src\\jsMain\\generated",
  "libraryName" : "blueprintjs-icons",
  "plugins" : "C:\\Projects\\kotlin-blueprintjs\\build\\js\\packages\\kotlin-blueprintjs-kotlin-blueprintjs-icons\\karakum/plugins/*.js",
  "annotations" : "C:\\Projects\\kotlin-blueprintjs\\build\\js\\packages\\kotlin-blueprintjs-kotlin-blueprintjs-icons\\karakum/annotations/*.js",
  "nameResolvers" : "C:\\Projects\\kotlin-blueprintjs\\build\\js\\packages\\kotlin-blueprintjs-kotlin-blueprintjs-icons\\karakum/nameResolvers/*.js",
  "inheritanceModifiers" : "C:\\Projects\\kotlin-blueprintjs\\build\\js\\packages\\kotlin-blueprintjs-kotlin-blueprintjs-icons\\karakum/inheritanceModifiers/*.js"
}

The resulting files are generated under:

src/jsMain/generated/blueprintjsiconsCProjectskotlinblueprintjsbuildjsnode_modulesblueprintjsiconslibesm

I'm assuming we're taking the last path element somewhere by relying on / separator, and on Windows this means the last segment is everything.

sgrishchenko commented 1 year ago

I hope with new update for Windows this problem will disappear, can you check it please?

joffrey-bion commented 1 year ago

Thanks! I will have a try maybe this weekend

joffrey-bion commented 1 year ago

Hi again! I cannot reproduce this specific issue fairly because the generated config is overwritten by Gradle now, forcing a slash-separated-but-Windowsian path (see https://github.com/karakum-team/karakum/issues/6#issuecomment-1649416389).

But from the new generated config, I get a file in src/jsMain/generated/index.kt, with a blueprintjs.icons package. Not sure if this is the intended path. Shouldn't it be under src/jsMain/generated/blueprintjs/icons/index.kt?

sgrishchenko commented 1 year ago

Shouldn't it be under src/jsMain/generated/blueprintjs/icons/index.kt?

It was made according to Kotlin Styleguide. image

If you want src/jsMain/generated/blueprintjs/icons/index.kt path, just use libraryNameOutputPrefix: true in you karakum.config.js

sgrishchenko commented 1 year ago

Can I close this ticket?

joffrey-bion commented 1 year ago

It was made according to Kotlin Styleguide.

I've never seen this recommendation followed. In fact, from my experience, using this approach breaks a bunch of refactorings in IDEA, so I've always reverted to the usual Java convention.

In any case, if it's overridable in the config, I'm good with it. Although the name libraryNameOutputPrefix seems strange. Does it affect other things than the path? If it doesn't affect anything else, I would suggest a name like useDirectoriesForRootPackage or rootPackageInNestedDirectories (or the reverse boolean: rootPackageInSourceRoot). IMO it isn't really linked to the library name, in the sense that the library name is used as root package anyway regardless of this setting. It's all about whether we follow the root-package-in-source-root convention or not.

sgrishchenko commented 1 year ago

libraryNameOutputPrefix seems strange. Does it affect other things than the path?

No, it just adds part of package, that was generated from libraryName and path prefix for output.

it isn't really linked to the library name

Maybe it is naming problem, but libraryName linked with linked with package prefix directly. libraryName is used for 2 things:

1) base package (and possibly base output path) 2) base module name in @JsModule

Maybe I need to consider other names for libraryName and libraryNameOutputPrefix, thanks for recommendations, I will try to improve naming.

sgrishchenko commented 1 year ago

Maybe I will invert defaults and name config option as stripOutputPrefix? Actually I want to avoid complex names in config to make it compact.

joffrey-bion commented 1 year ago

I see several problems here. The "output" of Karakum is a set of kotlin files. It's not a path. So a prefix for the output would rather mean something like a header prepended to every generated file (at least in my opinion). The output's root directory path is what we want to change here. That's why I don't like using the term "output" alone to refer to the root directory path of the generated files.

So I'd suggest stripRootPackageDirectories, or something along those lines.

About the library name, I think we must consider 2 separate concepts here: the root package for the generated code, and the module names for the JsModule annotations (that will affect how kotlin generates imports/require). The Kotlin package name is completely independent from the module names and it is just set by default to a dashified version of the library name. But that's just a default, it could be anything. It could even be customizable in the config, e.g. with a parameter called rootPackage.

So any parameter affecting whether we use directories for the root package or directly the source root shouldn't be related to the library name. The default value for the root package is irrelevant for such a parameter.

By the way I totally agree it would be great if we reversed the default of the param, if we're ok to slightly break existing projects using Karakum.

sgrishchenko commented 1 year ago

The Kotlin package name is completely independent from the module names ... But that's just a default, it could be anything.

That is why there are two config options: 1) moduleNameMapper for customization of paths in @JsModule annotation 2) packageNameMapper for customization of Kotlin packages

Yea, you are right, labraryName is way to provide defaults, but using packageNameMapper you can provide any Kotlin package layout you want (theoretically :slightly_smiling_face:).

joffrey-bion commented 1 year ago

using packageNameMapper you can provide any Kotlin package layout you want (theoretically 🙂)

Ah thanks, so even better. Then it clearly supports my point, which is that the name of option for stripping the root package directories shouldn't refer to the "library name".

What do you think about the name stripRootPackageDirectories? No matter if and how the user customizes the package names, this param allows to strip the directories that correspond to the root package (the common package prefix between all files).

By the way, I'm sorry I think I should have opened a totally separate issue!

sgrishchenko commented 1 year ago

I would like to close this issue, fill free to propose configuration improvement in additional issues.