Skip to content

fix variable/method names in avdl - #4926

Closed
songgao wants to merge 2 commits into
masterfrom
songgao/avdl-fix-1121
Closed

songgao wants to merge 2 commits into
masterfrom
songgao/avdl-fix-1121

Conversation

@songgao

@songgao songgao commented Nov 22, 2016

Copy link
Copy Markdown
Contributor

No description provided.

@songgao

songgao commented Nov 22, 2016

Copy link
Copy Markdown
Contributor Author

r? @mmaxim @gabriel

@maxtaco

maxtaco commented Nov 22, 2016

Copy link
Copy Markdown
Contributor

This change is going to be very challenging to roll out without breaking
chat. All legacy clients will have failures.

On Tue, Nov 22, 2016 at 10:40 AM Mike Maxim notifications@github.com
wrote:

@mmaxim approved this pull request.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#4926 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA05_8pKpP4OLejEXxUcjpZnDXP9_T3Mks5rAwlGgaJpZM4K44lC
.

@songgao

songgao commented Nov 22, 2016

Copy link
Copy Markdown
Contributor Author

Is it because of the codec name changes? Names in Go code doesn't change at all.

@maxtaco

maxtaco commented Nov 23, 2016

Copy link
Copy Markdown
Contributor

Wire representation is different and you are going to have rpc procedure
not found errors.

On Tue, Nov 22, 2016 at 11:24 AM Song Gao notifications@github.com wrote:

Is it because of the codec name changes? Names in Go code doesn't change
at all.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#4926 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA05_5CjZlT66O2uHWv517RY6JJlv0Wwks5rAxdJgaJpZM4K44lC
.

@songgao

songgao commented Nov 23, 2016

Copy link
Copy Markdown
Contributor Author

I see. So shall we drop this for now then?

@songgao

songgao commented Nov 23, 2016

Copy link
Copy Markdown
Contributor Author

Although on the other hand we haven't released chat yet ...

@maxtaco

maxtaco commented Nov 23, 2016

Copy link
Copy Markdown
Contributor

If it matters then it should be done now but everything will break when it
goes in.

Once chat is live we never can make breaking changes like this one.

On Tue, Nov 22, 2016 at 7:55 PM Song Gao notifications@github.com wrote:

Although on the other hand we haven't released chat yet ...


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#4926 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA05_0W4vcKXmH45h3Xt4SIBruHZnNgHks5rA48GgaJpZM4K44lC
.

@songgao

songgao commented Dec 2, 2016

Copy link
Copy Markdown
Contributor Author

closing in favor of #5004

@songgao songgao closed this Dec 2, 2016
@songgao
songgao deleted the songgao/avdl-fix-1121 branch December 2, 2016 19:07
@gabriel

gabriel commented Dec 2, 2016

Copy link
Copy Markdown
Contributor

We could do it Monday when everyone is together? If not, we're stuck with it forever :(

@songgao

songgao commented Dec 2, 2016

Copy link
Copy Markdown
Contributor Author

Yeah; sounds good!

@patrickxb

Copy link
Copy Markdown
Contributor

To be clear, there are users using chat outside the company even though it hasn't been released. This will likely cause them issues.

@songgao

songgao commented Dec 2, 2016

Copy link
Copy Markdown
Contributor Author

Yeah but it's not even released, and this is the same level of break that we've done in the past for chat since the soft launch. If we ever want the name fixed, now is a better time tha later unless we count on a V2 in the future.

@maxtaco

maxtaco commented Dec 2, 2016

Copy link
Copy Markdown
Contributor

@malgorithms says it's OK to break chat at the moment...

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.

5 participants