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 notices to functions that will go away in v0.3.0. #798

Merged
merged 1 commit into from Feb 23, 2018

Conversation

Projects
None yet
7 participants
@iphydf
Member

iphydf commented Feb 19, 2018

This change is Reviewable

@iphydf iphydf added this to the v0.2.0 milestone Feb 19, 2018

@zoff99

This comment has been minimized.

zoff99 commented Feb 19, 2018

what if you miss any events? why are all these going to be removed?

@iphydf

This comment has been minimized.

Member

iphydf commented Feb 19, 2018

How do you miss events?

@zoff99

This comment has been minimized.

zoff99 commented Feb 19, 2018

a bug (100% bugfree c-toxcore? wow),
or just dont want events but rather "ask" later about some thing

also what positive things are achieved by removing these functions?

@iphydf

This comment has been minimized.

Member

iphydf commented Feb 19, 2018

If toxcore fails to emit the callback, then most likely the value is also not stored inside the tox instance, so the getter doesn't give you anything more. The positive thing is that toxcore no longer needs to keep track of this information that is most likely stored in the client anyway. I'd like to make toxcore itself more "core"-ish, smaller interface, and provide additional functionality through extra libraries. Separation of concerns and all that :).

@robinlinden

This comment has been minimized.

Member

robinlinden commented Feb 19, 2018

:lgtm_strong:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@iphydf

This comment has been minimized.

Member

iphydf commented Feb 21, 2018

Added client maintainers to see if they can accept this change.

@zoff99

This comment has been minimized.

zoff99 commented Feb 21, 2018

LGTM.
yet i don't agree with the change to remove api call for clients to get some infos. having only the events is not good. more complex clients code.

@iphydf

This comment has been minimized.

Member

iphydf commented Feb 21, 2018

I agree, it makes the client code of very simple clients a bit more complex. I don't think it'll do much for already complex clients, because they will have found no use in getting e.g. the byte representation of the status message from scratch each time. They will have stored it in their own data structure in a form they can handle nicely (e.g. as UTF-16 string). I'm vaguely planning to pull out a bit of functionality from toxcore and put it in a client helper library for C clients (probably not very useful for other languages). That could store things like that and also be allowed file I/O, which toxcore isn't allowed.

@sudden6

This comment has been minimized.

sudden6 commented Feb 22, 2018

:lgtm_strong:


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@@ -443,7 +455,9 @@ static class options {
* @deprecated The memory layout of this struct (size, alignment, and field
* order) is not part of the ABI. To remain compatible, prefer to use $new to
* allocate the object and accessor functions to set the members. The struct
* will become opaque (i.e. the definition will become private) in v0.2.0.
* will become opaque (i.e. the definition will become private) in v0.2.x.

This comment has been minimized.

@nurupo

nurupo Feb 22, 2018

Member

API/ABI breaking change in v0.2.x? I thought that our versioning was that of Semantic Versioning but with the leading zero removed. Making such change in a v0.2.x instead of 0.2.0 or 0.3.0 violates that.

@nurupo

nurupo approved these changes Feb 22, 2018

@zoff99

This comment has been minimized.

zoff99 commented Feb 22, 2018

:lgtm_strong:


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@zoff99

This comment has been minimized.

zoff99 commented Feb 22, 2018

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iphydf

This comment has been minimized.

Member

iphydf commented Feb 23, 2018

@JFreegman please take a look. I've removed the deprecation notices from all the string valued getters, i.e. things that can be useful to store in the tox state for now.

@JFreegman

This comment has been minimized.

Member

JFreegman commented Feb 23, 2018

:lgtm_strong:


Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iphydf iphydf merged commit 223745e into TokTok:master Feb 23, 2018

5 of 6 checks passed

ci/circleci Your tests are queued behind your running builds
Details
WIP ready for review
Details
code-review/reviewable 4/4 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@iphydf iphydf deleted the iphydf:deprecations branch Feb 23, 2018

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