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

Feature/customizable tsfmt and prettier version #363

Conversation

simschla
Copy link
Contributor

@simschla simschla commented Feb 19, 2019

Providing a way to customize prettier/tsfmt versions.

This fixes #362

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

This looks great! I have a few nitpicks I left inline about keeping old methods around for back-compat, also a few other minor blips.

Only substantive feedback is update CHANGES and README.

setFile("test.ts").toResource("npm/tsfmt/tsfmt/tsfmt.dirty");
gradleRunner().withArguments("--stacktrace", "spotlessApply").build();
assertFile("test.ts").sameAsResource("npm/tsfmt/tsfmt/tsfmt.clean");
}
Copy link
Member

Choose a reason for hiding this comment

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

Great tests!

- keeping backward compatibility
- concise naming
- provide javadoc to clarify not-self-explanatory methods
@simschla
Copy link
Contributor Author

This looks great! I have a few nitpicks I left inline about keeping old methods around for back-compat, also a few other minor blips.

Only substantive feedback is update CHANGES and README.

Thanks for the feedback, I tried to integrate all the changes you requested. CHANGES and README will be updated ASAP.

@simschla
Copy link
Contributor Author

CHANGES and README updated to reflect the changes.


public static final String NPM_PKG_TS = "typescriptVersion";

public static final String NPM_PKG_TSLINT = "tslintVersion";
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I think my feedback asking for these constants wasn't clear. I would expect that NPM_PKG_TSFMT would be equal to its npm package name, typescript-formatter. And the same for the other constants. The problem with the current approach is that there's no consistent mapping between the npm package name and the name that spotless is expecting. This gets even harder for users to debug because there's no error if you use an unexpected package name, it's just ignored silently.

If you use the actual npm package names, then the build file could look like this:

tsfmt([
  'typescript-formatter': '7.2.2',
  'typescript': '2.9.2',
  'tslint': '5.1.0'])

which would look very familiar to any user that has used npm. This also opens up an error-checking opportunity. What if the package.json just used the versions map as its devDependencies. Then users will have the option to add plugins if they want, and if they mistype a package name they'll get a loud descriptive error from npm about the unknown package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarifications!

If you use the actual npm package names, then the build file could look like this

That would make it really look like the package.json. One caveat of this would be, that we can no longer profit on the groovy goodness of named parameters.

The method public TypescriptFormatExtension tsfmt(String formatterVersion, String typescriptVersion, String tslintVersion) and public TypescriptFormatExtension tsfmt(Map<String, String> versions) would no longer look similar in calling:

tsfmt(['typescript-formatter': '7.2.2', 'typescript': '2.9.2'])
// vs
tsfmt(typescriptFormatter: '7.2.2', typescript: '2.9.2')

The Problem I see there is the hyphen-case in typescript-formatter. So if we go this way, we might have to drop the tsfmt(String formatterVersion, String typescriptVersion, String tslintVersion) method overload and only provide the map signature. Opinions?

This gets even harder for users to debug because there's no error if you use an unexpected package name, it's just ignored silently.

That, however, we could address by checking contents of the map keys vor validity.

What if the package.json just used the versions map as its devDependencies. Then users will have the option to add plugins if they want

Although this seems convenient, it might be very maintenance intensive - we will not be able to check the system with all combinations and plugins and therefore cannot guarantee any "this combination works" labels. (And we will probably not be able to help out users who try any combination of versions and plugins and do not succeed).

(I for myself tried getting prettier to run with the java plugin but had no success since the plugin was not compatible with the j2v8 runtime at the time of trying.)

How do you suggest going forward?

Copy link
Member

Choose a reason for hiding this comment

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

So if we go this way, we might have to drop the tsfmt(String formatterVersion, String typescriptVersion, String tslintVersion) method overload and only provide the map signature. Opinions?

That's one less method to document and maintain. Further, there's no chance a user can use this method without A) looking up the latest package version on NPM and B) looking up the spotless-specific name for that particular package, which is different for each package. If we adopt the "map as devDependencies plus defaults", then all the user needs to know is "this map becomes your npm deps, plus these default packages and versions if you don't specify them". Less for us to document, less for us to maintain, same easy defaults for most users, more flexibility for power users.

That, however, we could address by checking contents of the map keys for validity.

Yup. I'm fine with this, and it's easy to do. But it's not as easy as dumping this job onto npm, and npm is going to redo that check again anyway.

it might be maintenance intensive - we will not be able to check the system with all combinations and plugins and therefore cannot guarantee any "this combination works" labels

By allowing users to specify versions for the 3 packages, we're already opening ourselves up to that. So long as the defaults work out of the box, I think it's fine if users discuss amongst themselves "these versions don't work" on our issues page.

It's true that by giving users power, we increase the chance that they raise issues. I think it's important to see those issues as something interesting you have the choice to engage with, but not as a responsibility. Your contribution of tsfmt and prettier was a huge and valuable donation of your time, you didn't owe it to anybody but you did it anyway. This PR is this same. And if users have questions about incompatible combos in the future, it might be a chance to learn about an interesting new prettier plugin. But definitely don't take them as your responsibility. The issues are a public forum, it's fine for them to stay open without a response.

@simschla simschla force-pushed the feature/customizable-tsfmt-and-prettier-version branch from cb661b7 to d96245b Compare February 22, 2019 06:44
@nedtwigg
Copy link
Member

@simschla Whaddya think of this direct-passthrough approach? It looks about as easy to use, but we get the advantage that npm is doing error-checking for us, and power users can use plugins and their own forks of npm packages.

@simschla
Copy link
Contributor Author

Thanks for stepping in since I wasn't able to get back to this the last 2 weeks...

The current state LGTM. One improvement we could make is making sure, that at least the required packages are specified when using the map-approach (especially for tsfmt -> that way one cannot 'forget' to specify the tslint-version for example...). What do you think?

@nedtwigg
Copy link
Member

Only issue is if I want to use my-tslint-pkg, rather than the official one. Not a common usecase, but if you are missing some packages you get a pretty loud error.

@simschla
Copy link
Contributor Author

Only issue is if I want to use my-tslint-pkg, rather than the official one. Not a common usecase, but if you are missing some packages you get a pretty loud error.

Didn't think of that, but makes sense, yes. So then, I don't see why we shouldn't merge this :-)

@nedtwigg nedtwigg merged commit 9843130 into diffplug:master Mar 12, 2019
@simschla simschla deleted the feature/customizable-tsfmt-and-prettier-version branch June 2, 2020 14:58
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.

Possible to specify a newer version of prettier?
2 participants