Do not duplicate packages when running gazelle-update-repos #47
Conversation
| if equiv(s[first_free_position-1], s[curr]) { | ||
| // If the last added element is equivalent is considered one, | ||
| // then we move the cursor back so that the currently considered element | ||
| // replace the previously added one. | ||
| // This function is only keeping the last representative of the equivalence class | ||
| // because it is used to deal with packages having or not a version number, | ||
| // and in case both option are available, the version number should be kept. | ||
| first_free_position-- | ||
| } | ||
| s[first_free_position] = s[curr] | ||
| first_free_position++ |
There was a problem hiding this comment.
I find easier to follow the loop when not mutating the index.
| if equiv(s[first_free_position-1], s[curr]) { | |
| // If the last added element is equivalent is considered one, | |
| // then we move the cursor back so that the currently considered element | |
| // replace the previously added one. | |
| // This function is only keeping the last representative of the equivalence class | |
| // because it is used to deal with packages having or not a version number, | |
| // and in case both option are available, the version number should be kept. | |
| first_free_position-- | |
| } | |
| s[first_free_position] = s[curr] | |
| first_free_position++ | |
| if equiv(s[first_free_position-1], s[curr]) { | |
| s[first_free_position-1] = s[curr] | |
| } else { | |
| s[first_free_position] = s[curr] | |
| first_free_position++ | |
| } |
| n := len(l) | ||
| // strconv.Atoi converts a string to an integer, | ||
| // and potentially outputs an error message. | ||
| _, err := strconv.Atoi(strings.ReplaceAll(l[n-1], ".", "")) |
There was a problem hiding this comment.
There is a low risk of overflow here. Would it be straighter to check that all characters are either digits or dots instead?
facundominguez
left a comment
facundominguez
left a comment
There was a problem hiding this comment.
LGTM.
Does it solve #46, though? Existing entries in packages aren't fed to mapSortedStringKeys, I think. It is gazelle the piece that merges the old and the new lists.
Also, I think, gazelle_cabal would be less puzzling on error if it rejected package identifiers when they specify conflicting versions for the same package. The current behavior would be to ignore the identifier with the lexicographically smaller version.
| _, err := strconv.Atoi(strings.ReplaceAll(l[n-1], ".", "")) | ||
| // If the conversion succeeded, the error is 'nil'. | ||
| if err == nil { | ||
| matched, _ := regexp.MatchString(`^[0-9]+(\.[0-9]+)*$`, l[n-1]) |
There was a problem hiding this comment.
Maybe the regex can be compiled only once instead of doing it once per call?
Also, looks like regexp allows to implement all of this function with FindStringIndex.
e.g.
regexp.Regexp chopVersionNumberRegexp = regexp.Compile(`-[0-9]+(\.[0-9]+)*$`)
func chopVersionNumber(s string) string {
loc := chopVersionNumberRegexp.FindStringIndex(s)
if loc == nil {
return s
} else {
return s[:loc[0]]
}
} | default: | ||
| panic("Packages should be a list!") | ||
| } | ||
| print(list) |
There was a problem hiding this comment.
There are a few print statements that should be removed.
| case *bzl.StringExpr: | ||
| list = append(list, exprElem.Value) | ||
| default: | ||
| panic("Elements of packages should be string!") |
There was a problem hiding this comment.
| panic("Elements of packages should be string!") | |
| panic("Elements of packages should be strings!") |
| print("Case accessed") | ||
| var list []string | ||
| pack := r.Attr("packages") | ||
| switch expr := pack.(type) { |
There was a problem hiding this comment.
This code is probably better put in a function that extracts an array of strings from an attribute value. gazelle_haskell_modules has such a function.
| panic("Packages should be a list!") | ||
| } | ||
| print(list) | ||
| r.SetAttr("packages", listSortedStringKeys(list)) |
There was a problem hiding this comment.
Is reordering the strings necessary? If so, perhaps it should be mentioned in the README.
This addresses #46