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

Sorting, reversing and reverse sorting #353

Closed
wants to merge 2 commits into from

Conversation

gamma
Copy link

@gamma gamma commented Jun 16, 2021

The PR adds sorting, reversing and reverse sorting of object lists. I was using these function in my fork for some time, since docker-gen looked stale some time ago. Would be nice to add these functions.

@buchdag can you please have a look at it?

@buchdag
Copy link
Member

buchdag commented Aug 3, 2021

@gamma sorry for the delay, that looks nice and might be helpful, would you mind adding tests ?

@gamma
Copy link
Author

gamma commented Aug 3, 2021

Sure should 😄
I'm not sure though when I can do it. Please keep the PR open until then.

@buchdag
Copy link
Member

buchdag commented Aug 3, 2021

👍

Do you mind if I add test myself if I find the time at some point ?

@gamma
Copy link
Author

gamma commented Aug 3, 2021

Absolutely not. Please go ahead 🙏

@buchdag
Copy link
Member

buchdag commented Aug 9, 2021

@gamma I'll probably go in a slightly different direction for sortObjectsByKeys() in order to have a bit more extensibility:

type sortable interface {
	sort.Interface
	set(string, interface{}) error
	get() []interface{}
}

type sortableData struct {
	data []interface{}
}

func (s sortableData) get() []interface{} {
	return s.data
}

// method required to implement sort.Interface
func (s sortableData) Len() int { return len(s.data) }

// method required to implement sort.Interface
func (s sortableData) Swap(i, j int) { s.data[i], s.data[j] = s.data[j], s.data[i] }

type sortableByKey struct {
	sortableData
	key string
}

func (s *sortableByKey) set(funcName string, entries interface{}) (err error) {
	entriesVal, err := getArrayValues(funcName, entries)
	if err != nil {
		return
	}
	s.data = make([]interface{}, entriesVal.Len())
	for i := 0; i < entriesVal.Len(); i++ {
		s.data[i] = reflect.Indirect(entriesVal.Index(i)).Interface()
	}
	return
}

// method required to implement sort.Interface
func (s sortableByKey) Less(i, j int) bool {
	values := map[int]string{i: "", j: ""}
	for k := range values {
		if v := reflect.ValueOf(deepGet(s.data[k], s.key)); v.Kind() != reflect.Invalid {
			values[k] = v.Interface().(string)
		}
	}
	return values[i] < values[j]
}

// Generalized SortBy function
func generalizedSortBy(funcName string, entries interface{}, s sortable, reverse bool) (sorted []interface{}, err error) {
	err = s.set(funcName, entries)
	if err != nil {
		return nil, err
	}
	if reverse {
		sort.Stable(sort.Reverse(s))
	} else {
		sort.Stable(s)
	}
	return s.get(), nil
}

// sortObjectsByKeysAsc returns a sorted array of objects, sorted by object's key field in ascending order
func sortObjectsByKeysAsc(objs interface{}, key string) ([]interface{}, error) {
	s := &sortableByKey{key: key}
	return generalizedSortBy("sortObjsByKeys", objs, s, false)
}

// sortObjectsByKeysDesc returns a sorted array of objects, sorted by object's key field in descending order
func sortObjectsByKeysDesc(objs interface{}, key string) ([]interface{}, error) {
	s := &sortableByKey{key: key}
	return generalizedSortBy("sortObjsByKey", objs, s, true)
}

The idea is to be be able to

  • easily provide both ascending and descending (reverse) sort function
  • be able to add other sorting methods with new implementations of the sortable interface (working mostly on the get() and Less() methods)

also this check

if v := reflect.ValueOf(deepGet(s.data[k], s.key)); v.Kind() != reflect.Invalid {
	values[k] = v.Interface().(string)
}

Is needed because reflect.ValueOf(deepGet(s.data[k], s.key)).Interface().(string) will panic if the target key isn't set.

I think I'll probably open another PR and co-author the commit with you if that's ok with you.

@gamma
Copy link
Author

gamma commented Aug 9, 2021

Absolutely. Honestly I'm not at all into Go - I just put together what I thought would be working and it was OK for my case. Please feel free to take this direction - it makes sense for future extensions.

@buchdag
Copy link
Member

buchdag commented Oct 26, 2021

superseded by #382

@buchdag buchdag closed this Oct 26, 2021
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.

2 participants