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

Add deprecation warnings for cols/include_columns #19

Closed

Conversation

dariusj
Copy link
Contributor

@dariusj dariusj commented Mar 19, 2013

  • Also update docs and include test

@ribasushi
Copy link
Collaborator

Excellent - this is exactly what I had in mind. Some notes:

"Resultset specifies..." sounds odd, given that the callsite reported will be the search() containing the attr in question. Also when possible we try to stick to a single sentence for warnings/exceptions. A better wording (which is also more in line with other deprecation notices) is "Resultset attribute 'foo' has been deprecated - use 'bar' instead".

Your first carp_unique has an extra quote mark at the end.

Please also add a line abot these to Changes (user-visible changes)

I noticed the email you used in the author list is different than what you used to make the commit. Intentional or oversight or...?

Cheers and thanks!

@dariusj
Copy link
Contributor Author

dariusj commented Mar 19, 2013

  • "Resultset specifies" - I reused the same structure as a carp statement just below it - happy to change it to what you suggest though, it's more succinct and sounds better anyway.
  • Oops you're right about the extra quote mark!
  • I'll update the Changes file (not quite sure under what heading to put things...)
  • Yeah I'm doing this at work, I didn't bother changing my git config... probably should though.

I'll rebase my commits into one with a matching email address too.

Let me know if there's any other changes you'd like me to do.

Cheers!

* Also update docs and include test
@ribasushi
Copy link
Collaborator

Merged as 11343b34c, only change was the reorganizing of the CHANGES line (it is a user visible change so goes in "New/Changes")

Thank you for your work!

@ribasushi ribasushi closed this Mar 20, 2013
@dariusj
Copy link
Contributor Author

dariusj commented Mar 20, 2013

Glad to hear it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants