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

Fix time.Time not equals bug #951

Closed
wants to merge 5 commits into from

Conversation

senyast4745
Copy link
Contributor

Adding test

Summary

Fix assert.ObjectsAreEqual bug with not equals time.Time before and after serializing\deserializing

Changes

Added a check in ObectsAreEqual in case the arguments are a time object

Motivation

Fix bug to use test in docker

Related issues

Closes #950

Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. You still need to run go generate as I don't see the auto-generated files in this PR.

I also think you have made some mistake with the _codegen sub-module as there have been changes to its module files.

_codegen/go.mod Outdated Show resolved Hide resolved
assert/assertions.go Show resolved Hide resolved
assert/assertions_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

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

Thank you for your time and effort. This looks great now :)

@boyan-soubachov
Copy link
Collaborator

I've had to fix the TravisCI config. Do you mind rebasing the branch and then I can merge it?

@senyast4745
Copy link
Contributor Author

Of course I do not mind

@senyast4745
Copy link
Contributor Author

I can merge the master in my branch pulling up new changes

@boyan-soubachov
Copy link
Collaborator

Thank you, there still seem to be some conflicts, do you mind having a look again?

boyan-soubachov
boyan-soubachov previously approved these changes Jun 7, 2020
@senyast4745
Copy link
Contributor Author

Sorry, upstream has not been updated. The last commit pulled all the changes from testify

@senyast4745
Copy link
Contributor Author

Thanks in advance for the opportunity to contribute))

@boyan-soubachov
Copy link
Collaborator

Something is very weird, still getting errors due to conflicts:
image

Are you able to rebase and squash maybe? I promise I will merge it as soon as you do :)

@senyast4745
Copy link
Contributor Author

senyast4745 commented Jul 19, 2020

Hi, very sorry, I was very busy. I merge the latest changes from master to my branch. Unfortunately, the commits could not be squashed, I'm afraid to do a hard push or rebase

leoleoasd
leoleoasd previously approved these changes Jul 20, 2020
@boyan-soubachov
Copy link
Collaborator

Hi, very sorry, I was very busy. I merge the latest changes from master to my branch. Unfortunately, the commits could not be squashed, I'm afraid to do a hard push or rebase

If you rebase, resolve any conflicts and then force-push your branch you should be fine.

We can't merge this PR while there are conflicts :)

@senyast4745
Copy link
Contributor Author

Please show me in which files the conflicts occur, I will solve them locally, because I still don't understand a little how and where I can rebase and force-push

@senyast4745
Copy link
Contributor Author

should i rebase my branch to master?

@boyan-soubachov
Copy link
Collaborator

should i rebase my branch to master?

Yes, you'd rebase on master, something along the lines of

git rebase master
<resolve conflicts with your editor>
git push -f fork

where fork is the remote set-up to your fork.

This is a good and thorough guide on the general flow: https://blog.scottlowe.org/2015/01/27/using-fork-branch-git-workflow/

@senyast4745
Copy link
Contributor Author

I did rebase and force-push, as you asked

@leoleoasd
Copy link

It seems that the fork branch's commit is still messy. Maybe consider check out a new branch from master branch of stretchr's repo locally, re-do your changes on fork branch and force-push your new branch to origin/fork?

image

@leoleoasd
Copy link

Should we close this given that #979 fixed the git history issue?

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

Successfully merging this pull request may close these issues.

assert.ObjectsAreEqual work not correct with serialized\deserialized time.Time
4 participants