Pr/697 review updates #940
Conversation
|
@markwest51 thanks! can you rebase it? |
|
@knocte - resolved conflicts that appeared, not sure about removing _ from private vars though and also whether to call async methods prefix, we normally remove that as you can tell whether method is async by await call in preview but up to you |
|
There are still conflicts, did you really rebase? rebasing shouldn't imply new commits. |
|
Nevermind, I had the wrong merge option chosen. There are still things to fix in this PR but before I point them out, I gotta ask: did you test it? does it work well for you? |
|
@knocte not tested anything yet, was gonna wait till codebase is ready before doing that |
but the problem about doing that is that we may break it in the next review iteration hehe |
|
@knocte that's fine :-), feel free to point out the stuff you see, will amend later after work |
it's not fine, believe me, finding regressions in code that was working before, is tricky :P Please test it first, and I will continue with the review once we know this in fact brings value. |
|
update event now gets triggered on each update, for test need to figure out how to read saved messages. Tasks.
Is @merqlove able to provide input as it is their original working code. |
markwest51
force-pushed
the
pr/697-review-updates
branch
2 times, most recently
from
acd9f32 to
c064dcb
Compare
|
Apparently there still are conflicts. |
|
@knocte where are these conflicts ?, updates work fine from what i can see when running for the last hr, have added cancellation token to methods, inside the main one there are subscribed tasks and idle tasks not sure what the intention of this was. |
|
Github says "This branch cannot be rebased due to conflicts |
|
@markwest51 how have you rebased? |
|
@knocte i rebased to a previous commit on my branch to remove some information, did i need to rebase to master or something |
|
I guess so, otherwise Github would not let me merge. |
|
think i resolved conflicts by rebasing thru GitBash to master, looks like VS2017 is not that good with GitHub, do not merge yet though until tested again |
|
@knocte it is back working ok now - getting all updates coming through, was having problems earlier but run a send message test which gave me a socket error about previous connection being closed and then it is back working fine, not sure what that would be but it looked like socket connection was not working, was fine before all the rebasing so could be that. |
Man, everytime you face a problem like this, please, paste the |
|
Github is still telling me |
|
BTW if it's easier for you, before rebasing you could squash your commits into 1, first. That would make rebasing easier I guess |
pr updates amended pr review changes resolved merge conflicts updates from last night before rebase update on message test now passing removed nlog references and usage resolve conflicts from HEAD Reapply Events PR sochix#679 update on message test now passing removed nlog references and usage
markwest51
force-pushed
the
pr/697-review-updates
branch
from
ada61ba to
3e0e53b
Compare
|
@knocte are you able to check conflicts now, squashed commits using gitbash in the end rather than VS |
|
@knocte have done the git commands specified, says branch is up to date and rebased to your master as asked. |
|
ok seems no conflicts now! will do the last review today |
README.md
Outdated
| { | ||
| var peer = new TLInputPeerUser() { UserId = status.UserId }; | ||
| client.SendMessageAsync(peer, "Você está online.").Wait(); | ||
| } catch {} |
There was a problem hiding this comment.
please remove this empty catch
| while (!request.ConfirmReceived) | ||
| while (!request.ConfirmReceived) | ||
| { | ||
| var result = DecodeMessage((await transport.Receive(token).ConfigureAwait(false)).Body); | ||
|
|
||
| using (var messageStream = new MemoryStream(result.Item1, false)) | ||
| using (var messageReader = new BinaryReader(messageStream)) | ||
| using (var messageStream = new MemoryStream (result.Item1, false)) | ||
| using (var messageReader = new BinaryReader (messageStream)) |
There was a problem hiding this comment.
please revert unneeded whitespace changes like the above
|
@knocte all comments done and reviewed. Have added a comment on the test as have worked out why the updates were not being triggered, it was because the user needed re-authenticating, i was using the session user so there must be a expiration time. Soon as i done that all was working fine |
|
@knocte we all good ? |
|
@markwest51 hey Mark, sorry to drop the ball on this. Reason is I wanted to push the Layer Update (to 108) first, and later merge this PR, but I lacked the motivation for very long to finish the Layer update first. Today I've finished that and I pushed it here: https://github.com/nblockchain/TgSharp . In the following days I plan to port this PR to that repo, if you don't beat me to it. |
updated changes aside from nLog from pr/697