Refactor of the code base: Remove underscore functions and refactor a panic #59

Skarlso · 2022-08-16T15:22:36Z

What this PR does / why we need it:

Cleanup and replacing panics and error handling.

This is branched off from #57. I'll rebase once that's merged.

Which issue(s) this PR fixes:
Related to #52. This doesn't close it yet.

I also removed the test package. It shouldn't be living with the codebase.

Special notes for your reviewer:

Release note:

gardener-robot · 2022-08-16T15:22:54Z

@Skarlso Thank you for your contribution.

yitsushi

AddKnownTypes() is in the repo, but in a different package, so technically it's en external call.

cmds/ocm/commands/ocmcmds/common/inputs/inputtype.go

pkg/contexts/config/core/configtypes.go

pkg/contexts/credentials/core/repotypes.go

pkg/contexts/oci/core/repotypes.go

pkg/contexts/ocm/core/repotypes.go

Co-authored-by: Balazs Nadasdi <[email protected]>

mandelsoft · 2022-08-24T14:16:45Z

pkg/contexts/config/core/configtypes.go

-func (t *configTypeScheme) AddKnowntypes(s ConfigTypeScheme) {
-	t.Scheme.AddKnownTypes(s)
-}
-


Why have you removed this. It is a useful interface function, even if it is not yet used.
It assures correct typing by overriding the inherited function and accepting the correct sub type.
And it therefore avoids the occurrence of a potential error.

I'm not following. This function is a lowerCase AddKnownTypes for some reason. It was never called from anything. Not even using reflection I did a search for the string. :)

You are right. It is intended for a library user who does not want do deal with errors. Because of its parameter type, calling the base method cannot cause an error. It assures the correct element type. And the only error reason cannot appear, so the errors code can be ignored.

Because Go does not support method overloading by parameter types, I had to choose another method type.

May be with Generics this will be better, but so far the lib is without generic, and I still would prefer to keep it this way.
There are so many places where Generics could simplify the code, but it would be a real redesign.

Basically I already tried some more complicated cases, but the Go generics solution failed to handle them correctly. There are two important things missing: upper AND lower bound operator, and a much better type inference.

mandelsoft · 2022-08-24T14:22:51Z

pkg/env/env.go

-type fsOpt struct {
-	dummyOption
-	path string
-	fs   vfs.FileSystem
-}
-
-func FileSystem(fs vfs.FileSystem, path string) fsOpt {
-	return fsOpt{
-		path: path,
-		fs:   fs,
-	}
-}
-
-func (o fsOpt) Mount(cfs *composefs.ComposedFileSystem) error {
-	return cfs.Mount(o.path, o.fs)
-}
-
-////////////////////////////////////////////////////////////////////////////////
-


This is a valid interface function of the lib.

Sure. Completely unused by anything.

That's why I removed it. It clutters the code base. Unless there is some reference somewhere to this?

It is in version control, so once we DO need for something, we can always just add it back in. :)

It is intended as a library, Therefore is natural, that not all methods/functions are already used. The included CLI code is just one client of the lib, but not necessarily the only one.

Hmm, if it is intended to be used as a library, this should be made clear that this is an open option. Either through documentation or through other means, such as usage or packaging that surfaces this.

But that's a different problem. I can put this back in. :)

mandelsoft · 2022-08-24T14:24:41Z

cmds/ocm/commands/ocicmds/artefacts/get/cmd.go

-	"tree": get_tree,
+var outputs = output.NewOutputs(getRegular, output.Outputs{
+	"wide": getWide,
+	"tree": getTree,
 }).AddChainedManifestOutputs(OutputChainFunction())


If you urgently want to change this, it should be changed everywhere. There are several commands following this pattern.

I change these as I encounter them. This PR is not concerned with that. However, I can open a separate PR just with naming refactors. :)

Yes, that seems to be better. I tried to keep the codebase somehow consistent. So if we change these method names (no problem with that :-) ). we should change it in all commands, because these methods have the same meaning and name in all those commands.

Ok, will do that ( open a separate PR ).

cmds/ocm/commands/ocmcmds/common/inputs/inputtype.go

mandelsoft · 2022-08-29T09:00:28Z

cmds/ocm/commands/ocicmds/artefacts/get/cmd.go

-	"tree": get_tree,
+var outputs = output.NewOutputs(getRegular, output.Outputs{
+	"wide": getWide,
+	"tree": getTree,
 }).AddChainedManifestOutputs(OutputChainFunction())


Yes, that seems to be better. I tried to keep the codebase somehow consistent. So if we change these method names (no problem with that :-) ). we should change it in all commands, because these methods have the same meaning and name in all those commands.

mandelsoft · 2022-08-29T09:07:29Z

pkg/contexts/config/core/configtypes.go

-func (t *configTypeScheme) AddKnowntypes(s ConfigTypeScheme) {
-	t.Scheme.AddKnownTypes(s)
-}
-


You are right. It is intended for a library user who does not want do deal with errors. Because of its parameter type, calling the base method cannot cause an error. It assures the correct element type. And the only error reason cannot appear, so the errors code can be ignored.

Because Go does not support method overloading by parameter types, I had to choose another method type.

May be with Generics this will be better, but so far the lib is without generic, and I still would prefer to keep it this way.
There are so many places where Generics could simplify the code, but it would be a real redesign.

Basically I already tried some more complicated cases, but the Go generics solution failed to handle them correctly. There are two important things missing: upper AND lower bound operator, and a much better type inference.

pkg/contexts/config/core/configtypes.go

mandelsoft · 2022-08-29T09:21:35Z

pkg/env/env.go

-type fsOpt struct {
-	dummyOption
-	path string
-	fs   vfs.FileSystem
-}
-
-func FileSystem(fs vfs.FileSystem, path string) fsOpt {
-	return fsOpt{
-		path: path,
-		fs:   fs,
-	}
-}
-
-func (o fsOpt) Mount(cfs *composefs.ComposedFileSystem) error {
-	return cfs.Mount(o.path, o.fs)
-}
-
-////////////////////////////////////////////////////////////////////////////////
-


It is intended as a library, Therefore is natural, that not all methods/functions are already used. The included CLI code is just one client of the lib, but not necessarily the only one.

Skarlso · 2022-08-29T11:53:36Z

cmds/ocm/pkg/processing/container.go

@@ -1,295 +0,0 @@
-// Copyright 2022 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file


@mandelsoft Let's remove this if you don't mind, it's completely commented out.

mandelsoft · 2022-08-29T12:12:56Z

cmds/ocm/pkg/processing/sequential.go

 func process(op operation) processing {
 	return func(it data.Iterable) data.Iterable {
-		slice := []interface{}{}
+		var slice []interface{}


are you sure the using code works with a nil array, because this is the difference you produce with this change

Reverted this.

* Deleted a bunch of things which significantly sped up build time * Apply suggestions from code review Co-authored-by: Balazs Nadasdi <[email protected]> * Updated error message and added expected type information * Putting back the removed stuff * Renaming funcs with underscore in it. Co-authored-by: Gergely Brautigam <[email protected]> Co-authored-by: Balazs Nadasdi <[email protected]>

Skarlso requested a review from a team as a code owner August 16, 2022 15:22

Skarlso requested review from robertgraeff and enrico-kaack-comp and removed request for a team August 16, 2022 15:22

gardener-robot added the needs/review label Aug 16, 2022

gardener-robot added size/xl labels Aug 16, 2022

Skarlso changed the title ~~Slowly remove panics from the codebase and clean up some left over and unused files~~ [WIP] Slowly remove panics from the codebase and clean up some left over and unused files Aug 16, 2022

Skarlso requested a review from yitsushi August 17, 2022 09:52

Skarlso force-pushed the remove-panics branch from 37347a1 to bc42f86 Compare August 17, 2022 10:04

Skarlso changed the title ~~[WIP] Slowly remove panics from the codebase and clean up some left over and unused files~~ [WIP] Slowly remove panics from the codebase Aug 17, 2022

Skarlso requested a review from mandelsoft August 17, 2022 12:16

Skarlso force-pushed the remove-panics branch from bc42f86 to 4a32387 Compare August 18, 2022 09:02

Skarlso mentioned this pull request Aug 19, 2022

remove panics from the codebase: part 2 #62

Closed

Deleted a bunch of things which significantly sped up build time

bcbadfc

Skarlso force-pushed the remove-panics branch from 4a32387 to bcbadfc Compare August 19, 2022 11:09

gardener-robot added size/m Medium and removed size/xl labels Aug 19, 2022

yitsushi reviewed Aug 19, 2022

View reviewed changes

Apply suggestions from code review

e057b69

Co-authored-by: Balazs Nadasdi <[email protected]>

yitsushi approved these changes Aug 19, 2022

View reviewed changes

Skarlso changed the title ~~[WIP] Slowly remove panics from the codebase~~ remove panics from the codebase: part 1 Aug 19, 2022

mandelsoft requested changes Aug 24, 2022

View reviewed changes

Updated error message and added expected type information

f92e07b

Skarlso requested a review from mandelsoft August 29, 2022 07:47

mandelsoft reviewed Aug 29, 2022

View reviewed changes

Skarlso force-pushed the remove-panics branch from 0432f45 to 8de46ad Compare August 29, 2022 11:14

Putting back the removed stuff

7871e47

Skarlso force-pushed the remove-panics branch from 8de46ad to 7871e47 Compare August 29, 2022 11:16

gardener-robot added size/xl and removed size/m Medium labels Aug 29, 2022

Skarlso changed the title ~~remove panics from the codebase: part 1~~ Refactor of the code base: Remove underscore functions and refactor a panic Aug 29, 2022

Skarlso commented Aug 29, 2022

View reviewed changes

mandelsoft reviewed Aug 29, 2022

View reviewed changes

Renaming funcs with underscore in it.

3b93b44

Skarlso force-pushed the remove-panics branch from 2c5614f to 3b93b44 Compare August 29, 2022 12:26

mandelsoft self-requested a review August 29, 2022 13:07

mandelsoft approved these changes Aug 29, 2022

View reviewed changes

Skarlso merged this pull request into main Aug 29, 2022

Skarlso deleted the remove-panics branch August 29, 2022 13:09

gardener-robot added the status/closed label Aug 29, 2022

mandelsoft mentioned this pull request Aug 30, 2022

Missing content of #59 #79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of the code base: Remove underscore functions and refactor a panic #59

Refactor of the code base: Remove underscore functions and refactor a panic #59

Skarlso commented Aug 16, 2022

gardener-robot commented Aug 16, 2022

yitsushi left a comment

mandelsoft Aug 24, 2022

Skarlso Aug 24, 2022

mandelsoft Aug 29, 2022

mandelsoft Aug 24, 2022

Skarlso Aug 24, 2022

Skarlso Aug 29, 2022

mandelsoft Aug 29, 2022

Skarlso Aug 29, 2022

mandelsoft Aug 24, 2022

Skarlso Aug 25, 2022

mandelsoft Aug 29, 2022

Skarlso Aug 29, 2022

mandelsoft Aug 29, 2022

mandelsoft Aug 29, 2022

mandelsoft Aug 29, 2022

Skarlso Aug 29, 2022

mandelsoft Aug 29, 2022

Skarlso Aug 29, 2022

		@@ -1,295 +0,0 @@
		// Copyright 2022 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file

Refactor of the code base: Remove underscore functions and refactor a panic #59

Refactor of the code base: Remove underscore functions and refactor a panic #59

Conversation

Skarlso commented Aug 16, 2022

gardener-robot commented Aug 16, 2022

yitsushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment