Skip to content

Conversation

welch
Copy link
Contributor

@welch welch commented Nov 16, 2015

issue #5505 versionadded/versionchanged directives for new stuff in 0.18.

It was not clear to me if I should also add these at the file/module level for new files (I did).

@@ -28,6 +28,8 @@

Note: this script does not check any of the dependent C libraries; it only
operates on the Cython .pyx files.

.. versionadded:: 0.18
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is ever rendered / interpreted. It doesn't hurt either, though.

@amueller
Copy link
Member

thank you, this is very helpful. The main comment is that for new files, each public class or function in the file needs a versionadded (or versionchanged if it was moved, as the ones in the model_selection module)

@welch
Copy link
Contributor Author

welch commented Nov 17, 2015

comments addressed:

  • wording for various min_samples_* versionchanged -> Added float values for percentages.
  • versionadded moved into classes and functions for new files
  • versionchanged "moved from ..." for reorganized files in model_selection/

I wouldn't ordinarily squash mid-review, but it's tidier reading.

@@ -30,15 +30,23 @@ class NotFittedError(ValueError, AttributeError):
... print(repr(e))
... # doctest: +NORMALIZE_WHITESPACE +ELLIPSIS
NotFittedError('This LinearSVC instance is not fitted yet',)

.. versionadded:: 0.18
Copy link
Member

Choose a reason for hiding this comment

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

All the exceptions where actually all moved, but from different places...

@amueller
Copy link
Member

Thanks for all the changes. This looks good. It might be nice to say where the exceptions came from, but I don't feel strongly about it.

@welch
Copy link
Contributor Author

welch commented Nov 20, 2015

chased down the original exception class locations and noted in versionadded directives

@amueller
Copy link
Member

merged via #7403

@amueller amueller closed this Oct 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants