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

Make state comparison more generic #4190

Merged
merged 2 commits into from
May 23, 2017
Merged

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented May 3, 2017

For the state comparison FileStateOS was accessed directly. By having new prospector types there might be different state objects. This is a first step to remove the dependency on FileStateOS and do state comparison string based.


// IsEqual compares the state to an other state supporing stringer based on the unique string
func (s *State) IsEqual(c fmt.Stringer) bool {
return s.String() == c.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow relying on String() functions seems dangerous to me. Perhaps you could have the same logic, but based on a string id that is only computed once and stored in the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially had a ID() function for comparison. I switched it to String() because the Stringer interface already exist. But perhaps have a local interface with ID() string is a better choice? +1 on storing it locally and not generating it every time, as it should not change over time.

@ruflin ruflin added the in progress Pull request is currently in progress. label May 4, 2017
@ruflin ruflin mentioned this pull request May 10, 2017
14 tasks
@ruflin ruflin force-pushed the state-comparison branch from 12da5cc to 7bdef3d Compare May 15, 2017 13:12
@ruflin
Copy link
Contributor Author

ruflin commented May 15, 2017

New version pushed

For the state comparison FileStateOS was accessed directly. By having new prospector types there might be different state objects. This is a first step to remove the dependency on FileStateOS and do state comparison string based.
@ruflin ruflin force-pushed the state-comparison branch from 7bdef3d to 4862d71 Compare May 17, 2017 06:50
@@ -10,6 +10,7 @@ import (

// State is used to communicate the reading state of a file
type State struct {
Id string `json:"-"` // local unique id to make comparison more efficient
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
struct field Id should be ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this can't be ID as it otherwise conflicts with the ID() method.

@@ -33,6 +34,20 @@ func NewState(fileInfo os.FileInfo, path string, t string) State {
}
}

// String returns a unique id for the state as a string
Copy link
Collaborator

Choose a reason for hiding this comment

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

[golint] reported by reviewdog 🐶
comment on exported method State.ID should be of the form "ID ..."

For the state comparison FileStateOS was accessed directly. By having new prospector types there might be different state objects. This is a first step to remove the dependency on FileStateOS and do state comparison string based.
@ruflin ruflin force-pushed the state-comparison branch from 4862d71 to 0b1a5c0 Compare May 17, 2017 07:26
@ruflin ruflin removed the in progress Pull request is currently in progress. label May 17, 2017
@tsg tsg merged commit 747e5bf into elastic:master May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants