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: Accept undefined options and commits array #12

Merged
merged 1 commit into from Sep 18, 2017
Merged

Conversation

pvdlg
Copy link
Member

@pvdlg pvdlg commented Sep 17, 2017

@pvdlg pvdlg force-pushed the undefined-options branch 2 times, most recently from a7aa616 to ba5e811 Compare September 17, 2017 23:09
@gr2m
Copy link
Member

gr2m commented Sep 17, 2017

can you elaborate on what the underlying problem is and how this resolves it, so we have it for future reference?

@pvdlg
Copy link
Member Author

pvdlg commented Sep 17, 2017

Sure it seems that semantic-release call the plugins passing the pluginConfig parameter as undefined if no options is. I'm not sure if it's something specific to the test case (the test are really difficult to figure out for me) or if there is a case in which semantic-release can actually do that.

My understand was that due to index.js#L26, options.analyzeCommits should always be an object and never be undefined. But I might be wrong.

Anyway it's probably better for the plugin to handle the case of pluginOptions being undefined the same as if it was equal to {}.

@pvdlg
Copy link
Member Author

pvdlg commented Sep 17, 2017

It seems codecov is not working and I don't have the proper access. I sent you a request to let me access codecov for the orga.

@gr2m
Copy link
Member

gr2m commented Sep 18, 2017

I’ve granted access to codecov

@codecov-io
Copy link

codecov-io commented Sep 18, 2017

Codecov Report

Merging #12 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #12   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines          83     83           
=====================================
  Hits           83     83
Impacted Files Coverage Δ
lib/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a64f07...34aee9e. Read the comment docs.

@gr2m
Copy link
Member

gr2m commented Sep 18, 2017

This seems wrong to me. defaulting pluginConfig to {} is okay, but wouldn't other plugins fail as well?

The fact that no commits array is being passed doesn’t make sense though, does it? I think there is another problem here. I’d like to further investigate it. I can have a look myself over the next weekend, if you could have a look yourself, that’d be graet

@pvdlg
Copy link
Member Author

pvdlg commented Sep 18, 2017

I added the case for no commit just as a safety. It was not failing. I can remove it.
It seems semantic-release can pass undefined for pluginConfig for each default plugins (and only the default ones). It hasn't been detected before because:

  • The previous default plugins do not use pluginConfig
  • The new plugins use pluginConfig and before the default the plugin configuration was mandatory
  • We don't see the error in release-notes-generator because there is integration test in semantic-release for the post command

@gr2m
Copy link
Member

gr2m commented Sep 18, 2017

Okay the undefined pluginConfig makes sense. I would not default the commits array though, can change that?

@pvdlg
Copy link
Member Author

pvdlg commented Sep 18, 2017

Okay the undefined pluginConfig makes sense. I would not default the commits array though, can change that?

Sure. I just pushed the change

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.

None yet

3 participants