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

Add a command line option to make warnings fatal (Default off) #43

Merged
merged 4 commits into from Jan 20, 2015

Conversation

Projects
None yet
2 participants
@chrestomanci
Contributor

chrestomanci commented Jan 11, 2015

Fixes issue #40 (bug #93023 on RT)

Unit test updated to not expect warnings to be fatal. POD docs updated.

Add a command line option to make warnings fatal (Default off)
Fixes issue #40 (bug #93023 on RT)

Unit test updated to not expect warnings to be fatal. POD docs updated.
@xsawyerx

This comment has been minimized.

Show comment
Hide comment
@xsawyerx

xsawyerx Jan 11, 2015

Owner

The only thing I would change is removing the "not recommended" comment.

It's less about what whether you have dependencies, but more of whether others depend on you, which makes the comment incorrect.

I think making it non-default basically says we would prefer you wouldn't use fatal warnings.

Owner

xsawyerx commented Jan 11, 2015

The only thing I would change is removing the "not recommended" comment.

It's less about what whether you have dependencies, but more of whether others depend on you, which makes the comment incorrect.

I think making it non-default basically says we would prefer you wouldn't use fatal warnings.

@chrestomanci

This comment has been minimized.

Show comment
Hide comment
@chrestomanci

chrestomanci Jan 12, 2015

Contributor

I don't mind removing the "not recommended" comment, from the short options description, but I think that this option would benefit from a longer explanation somewhere else, with links to the blog posts from Peter Rabbitson and chromatic, to explain more fully why fatal warnings are a bad idea. Question is where?

In any case, I need to add an entry to the changelog about this change, as it does change the public API. I also need to add changelog entries for the other changes I have pushed recently.

Contributor

chrestomanci commented Jan 12, 2015

I don't mind removing the "not recommended" comment, from the short options description, but I think that this option would benefit from a longer explanation somewhere else, with links to the blog posts from Peter Rabbitson and chromatic, to explain more fully why fatal warnings are a bad idea. Question is where?

In any case, I need to add an entry to the changelog about this change, as it does change the public API. I also need to add changelog entries for the other changes I have pushed recently.

@xsawyerx

This comment has been minimized.

Show comment
Hide comment
@xsawyerx

xsawyerx Jan 12, 2015

Owner

We could also remove the default fatal warnings. I'm not adverse to that.

Owner

xsawyerx commented Jan 12, 2015

We could also remove the default fatal warnings. I'm not adverse to that.

@chrestomanci

This comment has been minimized.

Show comment
Hide comment
@chrestomanci

chrestomanci Jan 14, 2015

Contributor

Seeing as this change affects the default behaviour of the module, I think the change should be prominently documented somewhere. Also some people will think that the new default is wrong and that best practice is to make warnings fatal.

My thinking is to add a couple of paragraphs to the main POD for the module along the lines of:

"Older versions of this module generated code that made warnings fatal [...] This is no longer the default, see option [...], See blog posts [link] and [link] for reasons why." [Summary explanation]

Would you mind if I made a post to the CPAN PR challenge mailing list about this?

Contributor

chrestomanci commented Jan 14, 2015

Seeing as this change affects the default behaviour of the module, I think the change should be prominently documented somewhere. Also some people will think that the new default is wrong and that best practice is to make warnings fatal.

My thinking is to add a couple of paragraphs to the main POD for the module along the lines of:

"Older versions of this module generated code that made warnings fatal [...] This is no longer the default, see option [...], See blog posts [link] and [link] for reasons why." [Summary explanation]

Would you mind if I made a post to the CPAN PR challenge mailing list about this?

@xsawyerx

This comment has been minimized.

Show comment
Hide comment
@xsawyerx

xsawyerx Jan 14, 2015

Owner

Not at all, though I don't see how the mailing list could resolve this. It's not meant for "Module::Starter development", it's meant for pull request challenge discussions (about providing pull requests, not specific features/changes in a module).

Owner

xsawyerx commented Jan 14, 2015

Not at all, though I don't see how the mailing list could resolve this. It's not meant for "Module::Starter development", it's meant for pull request challenge discussions (about providing pull requests, not specific features/changes in a module).

@chrestomanci

This comment has been minimized.

Show comment
Hide comment
@chrestomanci

chrestomanci Jan 14, 2015

Contributor

I have removed the 'not recommended' comment from the POD of the command line option, and instead put a short explanation into the changes file, with a link to the blog post about it for a fuller explanaton.

Contributor

chrestomanci commented Jan 14, 2015

I have removed the 'not recommended' comment from the POD of the command line option, and instead put a short explanation into the changes file, with a link to the blog post about it for a fuller explanaton.

xsawyerx added a commit that referenced this pull request Jan 20, 2015

Merge pull request #43 from chrestomanci/Fatalize_warning_option
Add a command line option to make warnings fatal (Default off)

@xsawyerx xsawyerx merged commit ad939ec into xsawyerx:master Jan 20, 2015

@xsawyerx

This comment has been minimized.

Show comment
Hide comment
@xsawyerx

xsawyerx Jan 20, 2015

Owner

Great stuff! 👍

Owner

xsawyerx commented Jan 20, 2015

Great stuff! 👍

@chrestomanci chrestomanci deleted the chrestomanci:Fatalize_warning_option branch Jan 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment