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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Aug 29, 2022
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bcbadfc
Deleted a bunch of things which significantly sped up build time
Skarlso Aug 16, 2022
e057b69
Apply suggestions from code review
Skarlso Aug 19, 2022
f92e07b
Updated error message and added expected type information
Skarlso Aug 29, 2022
7871e47
Putting back the removed stuff
Skarlso Aug 29, 2022
3b93b44
Renaming funcs with underscore in it.
Skarlso Aug 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions cmds/ocm/commands/ocicmds/artefacts/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,13 @@ package artefacts
import (
"github.com/spf13/cobra"

"github.com/open-component-model/ocm/pkg/contexts/clictx"

"github.com/open-component-model/ocm/cmds/ocm/pkg/utils"

"github.com/open-component-model/ocm/cmds/ocm/commands/ocicmds/artefacts/describe"
"github.com/open-component-model/ocm/cmds/ocm/commands/ocicmds/artefacts/download"
"github.com/open-component-model/ocm/cmds/ocm/commands/ocicmds/artefacts/get"
"github.com/open-component-model/ocm/cmds/ocm/commands/ocicmds/artefacts/transfer"
"github.com/open-component-model/ocm/cmds/ocm/commands/ocicmds/names"
"github.com/open-component-model/ocm/cmds/ocm/pkg/utils"
"github.com/open-component-model/ocm/pkg/contexts/clictx"
)

var Names = names.Artefacts
Expand All @@ -35,6 +33,7 @@ func NewCommand(ctx clictx.Context) *cobra.Command {
cmd := utils.MassageCommand(&cobra.Command{
Short: "Commands acting on OCI artefacts",
}, Names...)

cmd.AddCommand(get.NewCommand(ctx, get.Verb))
cmd.AddCommand(describe.NewCommand(ctx, describe.Verb))
cmd.AddCommand(transfer.NewCommand(ctx, transfer.Verb))
Expand Down
24 changes: 12 additions & 12 deletions cmds/ocm/commands/ocicmds/artefacts/get/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,24 +104,24 @@ func TableOutput(opts *output.Options, mapping processing.MappingFunction, wide
}
}

var outputs = output.NewOutputs(get_regular, output.Outputs{
"wide": get_wide,
"tree": get_tree,
var outputs = output.NewOutputs(getRegular, output.Outputs{
"wide": getWide,
"tree": getTree,
}).AddChainedManifestOutputs(OutputChainFunction())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


func get_regular(opts *output.Options) output.Output {
return closureoption.TableOutput(TableOutput(opts, map_get_regular_output)).New()
func getRegular(opts *output.Options) output.Output {
return closureoption.TableOutput(TableOutput(opts, mapGetRegularOutput)).New()
}

func get_wide(opts *output.Options) output.Output {
return closureoption.TableOutput(TableOutput(opts, map_get_wide_output, "MIMETYPE", "CONFIGTYPE")).New()
func getWide(opts *output.Options) output.Output {
return closureoption.TableOutput(TableOutput(opts, mapGetWideOutput, "MIMETYPE", "CONFIGTYPE")).New()
}

func get_tree(opts *output.Options) output.Output {
return output.TreeOutput(TableOutput(opts, map_get_regular_output), "NESTING").New()
func getTree(opts *output.Options) output.Output {
return output.TreeOutput(TableOutput(opts, mapGetRegularOutput), "NESTING").New()
}

func map_get_regular_output(e interface{}) interface{} {
func mapGetRegularOutput(e interface{}) interface{} {
digest := "unknown"
p := e.(*artefacthdlr.Object)
blob, err := p.Artefact.Blob()
Expand All @@ -142,12 +142,12 @@ func map_get_regular_output(e interface{}) interface{} {
return []string{p.Spec.UniformRepositorySpec.String(), p.Spec.Repository, kind, tag, digest}
}

func map_get_wide_output(e interface{}) interface{} {
func mapGetWideOutput(e interface{}) interface{} {
p := e.(*artefacthdlr.Object)

config := "-"
if p.Artefact.IsManifest() {
config = p.Artefact.ManifestAccess().GetDescriptor().Config.MediaType
}
return output.Fields(map_get_regular_output(e), p.Artefact.GetDescriptor().MimeType(), config)
return output.Fields(mapGetRegularOutput(e), p.Artefact.GetDescriptor().MimeType(), config)
}
17 changes: 9 additions & 8 deletions cmds/ocm/commands/ocmcmds/common/inputs/inputtype.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,6 @@ func NewInputTypeScheme(defaultRepoDecoder runtime.TypedObjectDecoder) InputType
return &inputTypeScheme{scheme}
}

func (t *inputTypeScheme) AddKnowntypes(s InputTypeScheme) {
t.Scheme.AddKnownTypes(s)
}

func (t *inputTypeScheme) GetInputType(name string) InputType {
d := t.GetDecoder(name)
if d == nil {
Expand All @@ -100,16 +96,21 @@ func (t *inputTypeScheme) GetInputType(name string) InputType {

func (t *inputTypeScheme) RegisterByDecoder(name string, decoder runtime.TypedObjectDecoder) error {
if _, ok := decoder.(InputType); !ok {
errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
return errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
}
return t.Scheme.RegisterByDecoder(name, decoder)
}

func (t *inputTypeScheme) AddKnownTypes(scheme runtime.Scheme) {
func (t *inputTypeScheme) AddKnownTypes(scheme runtime.Scheme) error {
if _, ok := scheme.(InputTypeScheme); !ok {
panic("can only add RepositoryTypeSchemes")
return errors.ErrInvalid("type", reflect.TypeOf(scheme).String(), "expected", "InputTypeScheme")
}
t.Scheme.AddKnownTypes(scheme)

if err := t.Scheme.AddKnownTypes(scheme); err != nil {
return fmt.Errorf("failed to add known type in inputTypeScheme: %w", err)
}

return nil
}

func (t *inputTypeScheme) Register(name string, rtype InputType) {
Expand Down
1 change: 0 additions & 1 deletion cmds/ocm/testhelper/forward.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,3 @@ import (
)

var TestData = env.TestData
var FileSystem = env.FileSystem
17 changes: 9 additions & 8 deletions pkg/contexts/config/core/configtypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,6 @@ func NewConfigTypeScheme(defaultRepoDecoder runtime.TypedObjectDecoder) ConfigTy
return &configTypeScheme{scheme}
}

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

func (t *configTypeScheme) GetConfigType(name string) ConfigType {
d := t.GetDecoder(name)
if d == nil {
Expand All @@ -68,16 +64,21 @@ func (t *configTypeScheme) GetConfigType(name string) ConfigType {

func (t *configTypeScheme) RegisterByDecoder(name string, decoder runtime.TypedObjectDecoder) error {
if _, ok := decoder.(ConfigType); !ok {
errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
return errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
}
return t.Scheme.RegisterByDecoder(name, decoder)
}

func (t *configTypeScheme) AddKnownTypes(scheme runtime.Scheme) {
func (t *configTypeScheme) AddKnownTypes(scheme runtime.Scheme) error {
if _, ok := scheme.(ConfigTypeScheme); !ok {
panic("can only add RepositoryTypeSchemes")
return errors.ErrInvalid("type", reflect.TypeOf(scheme).String(), "expected", "ConfigTypeScheme")
}
t.Scheme.AddKnownTypes(scheme)

if err := t.Scheme.AddKnownTypes(scheme); err != nil {
return fmt.Errorf("failed to add known type in config type scheme: %w", err)
}

return nil
mandelsoft marked this conversation as resolved.
Show resolved Hide resolved
}

func (t *configTypeScheme) Register(name string, rtype ConfigType) {
Expand Down
17 changes: 9 additions & 8 deletions pkg/contexts/credentials/core/repotypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,6 @@ func NewRepositoryTypeScheme(defaultRepoDecoder runtime.TypedObjectDecoder) Repo
return &repositoryTypeScheme{scheme}
}

func (t *repositoryTypeScheme) AddKnowntypes(s RepositoryTypeScheme) {
t.Scheme.AddKnownTypes(s)
}

func (t *repositoryTypeScheme) GetRepositoryType(name string) RepositoryType {
d := t.GetDecoder(name)
if d == nil {
Expand All @@ -70,16 +66,21 @@ func (t *repositoryTypeScheme) GetRepositoryType(name string) RepositoryType {

func (t *repositoryTypeScheme) RegisterByDecoder(name string, decoder runtime.TypedObjectDecoder) error {
if _, ok := decoder.(RepositoryType); !ok {
errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
return errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
}
return t.Scheme.RegisterByDecoder(name, decoder)
}

func (t *repositoryTypeScheme) AddKnownTypes(scheme runtime.Scheme) {
func (t *repositoryTypeScheme) AddKnownTypes(scheme runtime.Scheme) error {
if _, ok := scheme.(RepositoryTypeScheme); !ok {
panic("can only add RepositoryTypeSchemes")
return errors.ErrInvalid("type", reflect.TypeOf(scheme).String())
}

if err := t.Scheme.AddKnownTypes(scheme); err != nil {
return fmt.Errorf("failed to add known type: %w", err)
}
t.Scheme.AddKnownTypes(scheme)

return nil
}

func (t *repositoryTypeScheme) Register(name string, rtype RepositoryType) {
Expand Down
17 changes: 9 additions & 8 deletions pkg/contexts/oci/core/repotypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,6 @@ func NewRepositoryTypeScheme(defaultRepoDecoder runtime.TypedObjectDecoder) Repo
return &repositoryTypeScheme{scheme}
}

func (t *repositoryTypeScheme) AddKnowntypes(s RepositoryTypeScheme) {
t.Scheme.AddKnownTypes(s)
}

func (t *repositoryTypeScheme) GetRepositoryType(name string) RepositoryType {
d := t.GetDecoder(name)
if d == nil {
Expand All @@ -76,16 +72,21 @@ func (t *repositoryTypeScheme) GetRepositoryType(name string) RepositoryType {

func (t *repositoryTypeScheme) RegisterByDecoder(name string, decoder runtime.TypedObjectDecoder) error {
if _, ok := decoder.(RepositoryType); !ok {
errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
return errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
}
return t.Scheme.RegisterByDecoder(name, decoder)
}

func (t *repositoryTypeScheme) AddKnownTypes(scheme runtime.Scheme) {
func (t *repositoryTypeScheme) AddKnownTypes(scheme runtime.Scheme) error {
if _, ok := scheme.(RepositoryTypeScheme); !ok {
panic("can only add RepositoryTypeSchemes")
return errors.ErrInvalid("type", reflect.TypeOf(scheme).String(), "expected", "RepositoryTypeScheme")
}
t.Scheme.AddKnownTypes(scheme)

if err := t.Scheme.AddKnownTypes(scheme); err != nil {
return fmt.Errorf("failed to add known type in repository type scheme: %w", err)
}

return nil
}

func (t *repositoryTypeScheme) Register(name string, rtype RepositoryType) {
Expand Down
17 changes: 9 additions & 8 deletions pkg/contexts/ocm/core/repotypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,6 @@ func NewRepositoryTypeScheme(defaultRepoDecoder runtime.TypedObjectDecoder) Repo
return &repositoryTypeScheme{scheme}
}

func (t *repositoryTypeScheme) AddKnowntypes(s RepositoryTypeScheme) {
t.Scheme.AddKnownTypes(s)
}

func (t *repositoryTypeScheme) GetRepositoryType(name string) RepositoryType {
d := t.GetDecoder(name)
if d == nil {
Expand All @@ -80,16 +76,21 @@ func (t *repositoryTypeScheme) GetRepositoryType(name string) RepositoryType {

func (t *repositoryTypeScheme) RegisterByDecoder(name string, decoder runtime.TypedObjectDecoder) error {
if _, ok := decoder.(RepositoryType); !ok {
errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
return errors.ErrInvalid("type", reflect.TypeOf(decoder).String())
}
return t.Scheme.RegisterByDecoder(name, decoder)
}

func (t *repositoryTypeScheme) AddKnownTypes(scheme runtime.Scheme) {
func (t *repositoryTypeScheme) AddKnownTypes(scheme runtime.Scheme) error {
if _, ok := scheme.(RepositoryTypeScheme); !ok {
panic("can only add RepositoryTypeSchemes")
return errors.ErrInvalid("type", reflect.TypeOf(scheme).String(), "expected", "RepositoryTypeScheme")
}
t.Scheme.AddKnownTypes(scheme)

if err := t.Scheme.AddKnownTypes(scheme); err != nil {
return fmt.Errorf("failed to add known type in repository type scheme: %w", err)
}

return nil
}

func (t *repositoryTypeScheme) Register(name string, rtype RepositoryType) {
Expand Down
23 changes: 1 addition & 22 deletions pkg/env/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,11 @@ import (
"github.com/mandelsoft/vfs/pkg/readonlyfs"
"github.com/mandelsoft/vfs/pkg/vfs"

"github.com/open-component-model/ocm/pkg/contexts/datacontext/attrs/vfsattr"

"github.com/open-component-model/ocm/pkg/common/accessio"
"github.com/open-component-model/ocm/pkg/contexts/config"
"github.com/open-component-model/ocm/pkg/contexts/credentials"
"github.com/open-component-model/ocm/pkg/contexts/datacontext/attrs/vfsattr"
"github.com/open-component-model/ocm/pkg/contexts/oci"

"github.com/open-component-model/ocm/pkg/contexts/ocm"
)

Expand All @@ -45,25 +43,6 @@ func (dummyOption) Mount(*composefs.ComposedFileSystem) error {

////////////////////////////////////////////////////////////////////////////////

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)
}

////////////////////////////////////////////////////////////////////////////////

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid interface function of the lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Completely unused by anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

type tdOpt struct {
dummyOption
path string
Expand Down
7 changes: 4 additions & 3 deletions pkg/runtime/scheme.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func (t KnownTypes) TypeNames() []string {
// Scheme is the interface to describe a set of object types
// that implement a dedicated interface.
// As such it knows about the desired interface of the instances
// and can validate it. Additionally it provides an implementation
// and can validate it. Additionally, it provides an implementation
// for generic unstructured objects that can be used to decode
// any serialized from of object candidates and provide the
// effective type.
Expand All @@ -186,7 +186,7 @@ type Scheme interface {
Decode(data []byte, unmarshaler Unmarshaler) (TypedObject, error)
Encode(obj TypedObject, marshaler Marshaler) ([]byte, error)
EnforceDecode(data []byte, unmarshaler Unmarshaler) (TypedObject, error)
AddKnownTypes(scheme Scheme)
AddKnownTypes(scheme Scheme) error
KnownTypes() KnownTypes
KnownTypeNames() []string
}
Expand Down Expand Up @@ -243,12 +243,13 @@ func NewDefaultScheme(proto_ifce interface{}, proto_unstr Unstructured, acceptUn
}, nil
}

func (d *defaultScheme) AddKnownTypes(s Scheme) {
func (d *defaultScheme) AddKnownTypes(s Scheme) error {
d.lock.Lock()
defer d.lock.Unlock()
for k, v := range s.KnownTypes() {
d.types[k] = v
}
return nil
}

func (d *defaultScheme) KnownTypes() KnownTypes {
Expand Down