Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Aug 11, 2018

Since file handler name is being provided directly to Kaptan instance, it doesn't try to guess it. Although, it is able to do pretty much the same job of taking the extension from a filename and then using an appropriate file handler to parse stuff - you don't need to be so explicit here :)

As a fancy side-effect - for me, git-aggregator has finally started to parse .yml files properly. I'm aware that there's #14 in a master already, but that approach doesn't really solve the issue outside of the autocompletion scope.

Kaptan tries to guess the handler here
Currently, providing a handler='yml' handler directly ends up in a KeyError here

@lmignon
Copy link
Member

lmignon commented Aug 13, 2018

@arkostyuk Thank you for the contrib. Nevertheless your patch seems to break git-aggregator. Check travis.

@ghost
Copy link
Author

ghost commented Aug 13, 2018

ah, yes, didn't really noticed the part that passes a file content to Kaptan directly 🙂

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.711% when pulling 373f04c on arkostyuk:fix/really-allow-yml-extensions into 56d6710 on acsone:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.711% when pulling 373f04c on arkostyuk:fix/really-allow-yml-extensions into 56d6710 on acsone:master.

@coveralls
Copy link

coveralls commented Aug 13, 2018

Coverage Status

Coverage decreased (-0.9%) to 80.826% when pulling 2a23e26 on arkostyuk:fix/really-allow-yml-extensions into 56d6710 on acsone:master.

@ghost
Copy link
Author

ghost commented Aug 17, 2018

@lmignon the build is green now 😉

Copy link
Member

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

@arkostyuk Thank you LGTM

@sbidoul
Copy link
Member

sbidoul commented Aug 20, 2018

Looks good. Shall we do a release? @arkostyuk can you suggest a changelog entry?

@ghost
Copy link
Author

ghost commented Aug 21, 2018

@sbidoul which version should I target?

As it is possible to pass a file content instead of a file path to
Kaptan, a handler must be resolved before it happens. Though, it's
better to make an extra safety step and use Kaptan's own logic to
determine a suitable handler than to use bare file extension.
@sbidoul sbidoul merged commit c628284 into acsone:master Aug 21, 2018
@sbidoul
Copy link
Member

sbidoul commented Aug 21, 2018

@arkostyuk I tagged 1.3.0, it should reach pypi soon

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.

3 participants