Skip to content

chore: code-refactoring#85

Open
ShashwatDadhich wants to merge 24 commits intomainfrom
git-sensor-sync
Open

chore: code-refactoring#85
ShashwatDadhich wants to merge 24 commits intomainfrom
git-sensor-sync

Conversation

@ShashwatDadhich
Copy link
Contributor

@ShashwatDadhich ShashwatDadhich commented Feb 1, 2024

Refactoring done to make structs and interfaces extended

@ShashwatDadhich ShashwatDadhich changed the title Git sensor sync chore: code-refactoring Feb 1, 2024
Comment on lines 379 to 397
Copy link
Contributor

Choose a reason for hiding this comment

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

fix

Comment on lines 137 to 156
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this here?

Comment on lines 195 to 213
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed?

Comment on lines 78 to 87
Copy link
Contributor

Choose a reason for hiding this comment

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

need to have a look

Copy link
Contributor

Choose a reason for hiding this comment

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

why public

Copy link
Contributor

Choose a reason for hiding this comment

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

why public

Comment on lines 11 to 14
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Comment on lines 12 to 14
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

// }
type GoGitSDKManagerImpl struct {
*GitManagerBaseImpl
GitManagerBase
Copy link
Contributor

Choose a reason for hiding this comment

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

interface here but impl in cliManager. why?

Copy link
Contributor

Choose a reason for hiding this comment

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

can we make interfaces in both places?

Copy link
Contributor Author

@ShashwatDadhich ShashwatDadhich Feb 2, 2024

Choose a reason for hiding this comment

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

Yes sure....Used interface in cliManager as well

Copy link
Contributor

Choose a reason for hiding this comment

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

correct pls

import (
"context"
"github.com/devtron-labs/git-sensor/internal/sql"
"github.com/devtron-labs/git-sensor/internals/sql"

Choose a reason for hiding this comment

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

internal directory - is a special name that do not allow others to import any package under this directory. So after renaming it to internals - anyone can import this packages. I am almost sure that this is not a main goal of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments