Add method to remove property #96
Conversation
| var keptProperties []IANAProperty | ||
| for i := range cb.Properties { | ||
| if cb.Properties[i].IANAToken != string(removeProp) { | ||
| keptProperties = append(keptProperties, cb.Properties[i]) |
There was a problem hiding this comment.
Appending here we effectively make a new array, I think we should do an inplace remove. Completely unverified by me. Using AI I got:
i := 0
for _, prop := range cb.Properties {
if prop.IANAToken != string(removeProp) {
cb.Properties[i] = prop
i++
}
}
cb.Properties = cb.Properties[:i]
Probably a good idea to write a test for that too.
There was a problem hiding this comment.
I really prefer readability over premature optimization. For the above in-place attempt, I really have to think very carefully about its workings, and whether it might have some edge cases or such. My code is really straightforward to read and understand. Is making a new array really an issue performance wise here?
There was a problem hiding this comment.
It isn't a core concern of the library, as I found another area where I preference memory over performance.
I guess the real issue here is if people are using the slice directly. For simplicity I guess it's better to go with yours for now.
However there are lint issues remaining if you resync it should help.
One thing your version will do is put a nil slice instead of an empty slice. Which is different, but I don't know if it's a concern:
https://go.dev/play/p/iRgj5Jsqe8O
--
Looks like it really only matters for json and other reflection like matters. So non-issue.
|
Thanks. I have reviewed. The fix might solve the lint issue too. |
The lint issue is probably caused by the github action is not locked to a specific version of golangci-lint. So whenever things change in new version, the next run of the ci might trigger some lint errors that were not errors before. So we get unrelated lint complaints on PRs, like here. I'd suggest locking golangci-lint to a specific version, and fixing the lints on main. Then yes, bumping the linter will have to become a regular chore, but at least PRs won't be hit by random new lint rules. |
|
Added test though |
|
@quite I've merged a PR that conflicts with |
| var keptProperties []IANAProperty | ||
| for i := range cb.Properties { | ||
| if cb.Properties[i].IANAToken != string(removeProp) { | ||
| keptProperties = append(keptProperties, cb.Properties[i]) |
There was a problem hiding this comment.
It isn't a core concern of the library, as I found another area where I preference memory over performance.
I guess the real issue here is if people are using the slice directly. For simplicity I guess it's better to go with yours for now.
However there are lint issues remaining if you resync it should help.
One thing your version will do is put a nil slice instead of an empty slice. Which is different, but I don't know if it's a concern:
https://go.dev/play/p/iRgj5Jsqe8O
--
Looks like it really only matters for json and other reflection like matters. So non-issue.
quite
force-pushed
the
removeproperty
branch
from
0a291f4
to
e9a75cb
Compare
|
rebased, i think lint issues should be done |
|
When do you need this released? Soon, or you can wait for the #73 ? |
|
You mean it will return a nil slice if the last prop is removed right? I think that is fine, since in practice there is no difference between a nil slice and an empty slice. Go styleguide also: https://google.github.io/styleguide/go/decisions#nil-slices
…On 28 September 2024 09:03:19 CEST, Arran Ubels ***@***.***> wrote:
@arran4 approved this pull request.
Will have to reapprove once the lint issue is resolved. Comments applied
> @@ -89,6 +89,18 @@ func (cb *ComponentBase) AddProperty(property ComponentProperty, value string, p
cb.Properties = append(cb.Properties, r)
}
+// RemoveProperty removes from the component all properties that has
+// the name passed in removeProp.
+func (cb *ComponentBase) RemoveProperty(removeProp ComponentProperty) {
+ var keptProperties []IANAProperty
+ for i := range cb.Properties {
+ if cb.Properties[i].IANAToken != string(removeProp) {
+ keptProperties = append(keptProperties, cb.Properties[i])
It isn't a core concern of the library, as I found another area where I preference memory over performance.
I guess the real issue here is if people are using the slice directly. For simplicity I guess it's better to go with yours for now.
However there are lint issues remaining if you resync it should help.
One thing your version will do is put a nil slice instead of an empty slice. Which is different, but I don't know if it's a concern:
https://go.dev/play/p/iRgj5Jsqe8O
|
Yeah I had to verify that. It's maps which you aren't so lucky, kinda wished they were both one way or the other. |
Fixes #95