zrhoffman commented on code in PR #7718:
URL: https://github.com/apache/trafficcontrol/pull/7718#discussion_r1305979282
##########
traffic_ops/traffic_ops_golang/api/api.go:
##########
@@ -893,12 +893,21 @@ func (v *Version) String() string {
return strconv.FormatUint(v.Major, 10) + "." +
strconv.FormatUint(v.Minor, 10)
}
-func (v *Version) LessThan(otherVersion *Version) bool {
+// LessThan returns whether the version is strictly older than some other
+// version.
+func (v Version) LessThan(otherVersion Version) bool {
return v.Major < otherVersion.Major || (v.Major == otherVersion.Major
&& v.Minor < otherVersion.Minor)
}
-func (v *Version) GreaterThanOrEqualTo(otherVersion *Version) bool {
- return !v.LessThan(otherVersion)
+// GreaterThan returns whether the version is strictly newer than some other
+// version.
+func (v Version) GreaterThan(otherVersion Version) bool {
+ return v.Major > otherVersion.Major || (v.Major == otherVersion.Major
&& v.Minor > otherVersion.Minor)
+}
+
+// Equal returns whether the version is equal to the passed version.
+func (v Version) Equal(otherVersion Version) bool {
+ return v.Major == otherVersion.Major && v.Minor == otherVersion.Minor
}
Review Comment:
Using the `==` operator works for `Version` too, but since Go doesn't
support operator overloading, I guess it makes sense to have functions for all
comparisons instead of functions for some and operators for others.
##########
traffic_ops/traffic_ops_golang/cdn/cdns.go:
##########
@@ -317,7 +317,7 @@ func (cdn *TOCDN) ParamColumns()
map[string]dbhelpers.WhereColumnInfo {
"id": dbhelpers.WhereColumnInfo{Column: "id",
Checker: api.IsInt},
"name": dbhelpers.WhereColumnInfo{Column: "name"},
}
- if cdn.APIInfo().Version.GreaterThanOrEqualTo(&api.Version{Major: 4,
Minor: 1}) {
+ if !cdn.APIInfo().Version.LessThan(api.Version{Major: 4, Minor: 1}) {
Review Comment:
This is objectively harder to read and understand, please change it back to
`Version.GreaterThanOrEqualTo`
##########
traffic_ops/traffic_ops_golang/deliveryservice/eligible.go:
##########
@@ -98,7 +98,7 @@ func GetServersEligible(w http.ResponseWriter, r
*http.Request) {
}
// Based on version we load Delivery Service Eligible Server - for
version 5 and above we use DSServerV5
- if inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5, Minor: 0}) {
+ if !inf.Version.LessThan(api.Version{Major: 5, Minor: 0}) {
Review Comment:
Please change this back to `Version.GreaterThanOrEqualTo`
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -2281,12 +2281,12 @@ func Delete(inf *api.APIInfo) (int, error, error) {
}
inf.CreateChangeLog(fmt.Sprintf("SERVER: %s.%s, ID: %d, ACTION:
deleted", server.HostName, server.DomainName, server.ID))
- if inf.Version.Major >= 5 {
+ if !inf.Version.LessThan(api.Version{Major: 5}) {
return inf.WriteSuccessResponse(server, "Server deleted")
}
downgraded := server.Downgrade()
- if inf.Version.Major >= 4 {
+ if !inf.Version.LessThan(api.Version{Major: 4}) {
Review Comment:
Please use `Version.GreaterThanOrEqualTo` here
##########
traffic_ops/traffic_ops_golang/deliveryservice/keys.go:
##########
@@ -193,7 +193,7 @@ func GetSSLKeysByXMLID(w http.ResponseWriter, r
*http.Request) {
keyObjV4, err := getSslKeys(inf, r.Context())
if err != nil {
if err == sql.ErrNoRows {
- if inf.Version.GreaterThanOrEqualTo(&api.Version{Major:
5, Minor: 0}) {
+ if !inf.Version.LessThan(api.Version{Major: 5, Minor:
0}) {
Review Comment:
Please change this back to `Version.GreaterThanOrEqualTo`
##########
traffic_ops/traffic_ops_golang/deliveryservice/servers/servers.go:
##########
@@ -726,7 +726,7 @@ func GetReadAssigned(w http.ResponseWriter, r
*http.Request) {
}
// Based on version we load Delivery Service Server - for version 5 and
above we use DSServerV5
- if inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5, Minor: 0}) {
+ if !inf.Version.LessThan(api.Version{Major: 5, Minor: 0}) {
Review Comment:
Please change this back to `Version.GreaterThanOrEqualTo`
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -681,13 +681,13 @@ func Read(inf *api.APIInfo) (int, error, error) {
if userErr != nil || sysErr != nil {
return errCode, userErr, sysErr
}
- if version.Major >= 5 {
+ if !version.LessThan(api.Version{Major: 5}) {
Review Comment:
Please use `Version.GreaterThanOrEqualTo` here
##########
traffic_ops/traffic_ops_golang/cdn_lock/cdn_lock.go:
##########
@@ -104,7 +104,7 @@ func Read(w http.ResponseWriter, r *http.Request) {
api.HandleErr(w, r, tx, http.StatusInternalServerError,
nil, errors.New("scanning cdn locks: "+err.Error()))
return
}
- if inf.Version != nil &&
inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5, Minor: 0}) {
+ if inf.Version != nil &&
!inf.Version.LessThan(api.Version{Major: 5, Minor: 0}) {
Review Comment:
Please change this back to `Version.GreaterThanOrEqualTo`
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -1305,7 +1305,7 @@ func Update(inf *api.APIInfo) (int, error, error) {
var statusLastUpdatedTime time.Time
tx := inf.Tx.Tx
- if inf.Version.Major >= 5 {
+ if !inf.Version.LessThan(api.Version{Major: 5}) {
Review Comment:
Please use `Version.GreaterThanOrEqualTo` here
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -2281,12 +2281,12 @@ func Delete(inf *api.APIInfo) (int, error, error) {
}
inf.CreateChangeLog(fmt.Sprintf("SERVER: %s.%s, ID: %d, ACTION:
deleted", server.HostName, server.DomainName, server.ID))
- if inf.Version.Major >= 5 {
+ if !inf.Version.LessThan(api.Version{Major: 5}) {
Review Comment:
Please use `Version.GreaterThanOrEqualTo` here
##########
traffic_ops/traffic_ops_golang/logs/log.go:
##########
@@ -96,7 +96,7 @@ func Getv40(w http.ResponseWriter, r *http.Request) {
}
var result interface{}
var logsV5 []tc.LogV5
- if inf.Version.GreaterThanOrEqualTo(&api.Version{
+ if !inf.Version.LessThan(api.Version{
Review Comment:
Please change this back to `Version.GreaterThanOrEqualTo`
##########
traffic_ops/traffic_ops_golang/federation_resolvers/federation_resolvers.go:
##########
@@ -176,7 +176,7 @@ func Read(w http.ResponseWriter, r *http.Request) {
}
// Based on version we load types - for version 5 and above we
use FederationResolverV5
- if inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5,
Minor: 0}) {
+ if !inf.Version.LessThan(api.Version{Major: 5, Minor: 0}) {
Review Comment:
Please change this back to `Version.GreaterThanOrEqualTo`
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -1327,7 +1327,7 @@ func Update(inf *api.APIInfo) (int, error, error) {
return http.StatusBadRequest, userErr, sysErr
}
server = tmp.Upgrade()
- } else if inf.Version.Major >= 4 {
+ } else if !inf.Version.LessThan(api.Version{Major: 4}) {
Review Comment:
Please use `Version.GreaterThanOrEqualTo` here
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -926,7 +926,7 @@ JOIN server_profile sp ON s.id = sp.server`
if err != nil {
return nil, serverCount, nil, fmt.Errorf("getting
servers: %w", err), http.StatusInternalServerError, nil
}
- if (version.GreaterThanOrEqualTo(&api.Version{Major: 4}) &&
roleBasedPerms) || version.GreaterThanOrEqualTo(&api.Version{Major: 5}) {
+ if (!version.LessThan(api.Version{Major: 4}) && roleBasedPerms)
|| !version.LessThan(api.Version{Major: 5}) {
Review Comment:
This line is particularly harder to reason about without
`Version.GreaterThanOrEqualTo`, please change it back to that.
##########
traffic_ops/traffic_ops_golang/server/servers.go:
##########
@@ -1477,7 +1477,7 @@ func Update(inf *api.APIInfo) (int, error, error) {
where := `WHERE s.id = $1`
var selquery string
- if inf.Version.Major <= 4 {
+ if !inf.Version.GreaterThan(api.Version{Major: 4}) {
Review Comment:
Back to the original line in question: `!Version.GreaterThan` is not as easy
to reason about as `Version.LessThanOrEqualTo`. But if you feel strongly about
it, `!Version.GreaterThan` it's fine, since no `Version.LessThanOrEqualTo`
method already exists.
##########
traffic_ops/traffic_ops_golang/cdn/cdns.go:
##########
@@ -364,15 +364,15 @@ func (cdn *TOCDN) Validate() (error, error) {
"name": validation.Validate(cdn.Name,
validation.Required, validName),
"domainName": validation.Validate(cdn.DomainName,
validation.Required, validDomainName),
}
- if cdn.APIInfo().Version.GreaterThanOrEqualTo(&api.Version{Major: 4,
Minor: 1}) {
+ if !cdn.APIInfo().Version.LessThan(api.Version{Major: 4, Minor: 1}) {
Review Comment:
Please change this back to `Version.GreaterThanOrEqualTo` also to keep it
easy to understand
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
