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

Means for constructing YAML lint config from input stream. #34

Closed
garretwilson opened this issue Nov 16, 2020 · 3 comments
Closed

Means for constructing YAML lint config from input stream. #34

garretwilson opened this issue Nov 16, 2020 · 3 comments
Milestone

Comments

@garretwilson
Copy link
Contributor

First of all thank you so much for accepting issue #20 and putting out a new release. We're just now integrating the new version into our project, and everything is looking wonderful so far. We've already found a duplicate key that was causing a bug in production we didn't even know about.

This new issue is similar both to issue #20 and also to issue #19. The YamlLintConfig class provides as way to parse a configuration from a string:

public YamlLintConfig(String content) throws YamlLintConfigException {
  …
}

But I couldn't find a way to construct a YamlLintConfig instance from an InputStream. This would be useful if we stored the config YAML file in the class resources, e.g.:

InputStream is = getClass().getResourceAsStream("my-yaml-lint-config.yaml");

A developer can load the resource into a String and pass it to the YamlLintConfig constructor above, but then the developer would have to make sure that they check for the BOM and use the correct charset as discussed in #19. It would be much better if there were a way to construct a YamlLintConfig from an InputStream.

In fact I don't think it's ideal to be doing I/O in the constructor. A better practice would be to have a static factory method, like this, in YamlLintConfig:

public static YamlLintConfig read(InputStream is) throws IOException, YamlLintConfigException {
  //TODO do I/O stuff, parse everything, and return a constructed YamlLintConfig
}

In fact even for parsing from a String, a more modern approach would be to do the parsing in a static factory method; the constructor should usually be reserved for initialization values that have already been obtained somewhere. (I used to put everything in the constructor, too, many years ago.) Something like this:

public static YamlLintConfig parse(CharSequence text) throws YamlLintConfigException {
  …
}

(I got fancy and threw in a CharSequence.) But I don't want to quibble—I'm just giving suggestions.

The important thing is that we need a way to get a YamlLintConfig from an input stream, and have that code correctly determine the charset as in issue #19 .

I personally don't need this any time soon — we are already reading from resources and doing the charset detection manually as in issue #19 . I'm just filing this issue to improve the library at some point in the future when you have time.

Cheers!

@sbaudoin
Copy link
Owner

Hello,

Glad to hear it helped.

Funnily enough, I was considering such an improvement last week-end, using for the moment a CharSequence instead of a String to be more generic but I quickly faced an issue: I need the indexOf() method that is not available on the CharSequence interface.

Anyway, why do you think this is not ideal to deal ith I/O's? I'm not opposed to it but for consistency reasons I would then have to transform the current constructors into static factory methods.

@sbaudoin
Copy link
Owner

@garretwilson You can review the PR #35. I managed to find a way to use CharSequences instead of Strings in the Linter and YamlLintConfig classes + I implemented a new constructor with InputStream for this latter class.

@sbaudoin
Copy link
Owner

Fixed by #35

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

No branches or pull requests

2 participants