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.

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

fix: rename package to match directory name #72

Closed
wants to merge 1 commit into from

Conversation

zaibon
Copy link
Contributor

@zaibon zaibon commented May 17, 2024

No description provided.

@auto-assign auto-assign bot requested a review from edoardottt May 17, 2024 14:39
@edoardottt edoardottt self-assigned this May 18, 2024
@edoardottt
Copy link
Owner

Hi @zaibon ! thanks for the PR.

I was wondering.. is there a specific reason for this change?
As far as I can see the user has to use a proper name to import the module:

import (
	"testing"

	"github.com/edoardottt/depsdev/pkg/client"
	def "github.com/edoardottt/depsdev/pkg/depsdev/definitions"
	"github.com/stretchr/testify/require"
)

See https://github.com/edoardottt/depsdev/blob/devel/pkg/depsdev/v3/api_internal_test.go#L21

How this will change?

thanks :)

@zaibon
Copy link
Contributor Author

zaibon commented May 18, 2024

Usually, the name of you package follow the name of the directory where it lives. It's not mandatory, but that's what you usually see.

At the moment, because both package are called depsdev. you cannot import both "github.com/edoardottt/depsdev/pkg/depsdev/v3" and "github.com/edoardottt/depsdev/pkg/depsdev/v3/definitions" without using a renamed import.

Actually, now that I'm looking at it. the package located at "github.com/edoardottt/depsdev/pkg/depsdev/v3" is also called depsdev. I've never seen an organization like this before.

@edoardottt
Copy link
Owner

I've never seen an organization like this before.

Suggest what you think it'd the best option :) I'm all ears!

I renamed the packages in a rush cuz I wanted to fit v3 and v3alpha in the same place.

@zaibon
Copy link
Contributor Author

zaibon commented May 21, 2024

I renamed the packages in a rush cuz I wanted to fit v3 and v3alpha in the same place.

Ok, I understand where this comes from now.

I've had a look at the code in the V3 and V3Alpha package, and at the moment the only difference seems to be the base path used when creating the APIV3 or APIV3alpha struct. There is no change nor in the logic or the types used to read the responses.
So what I would do is, get rid of the V3 and V3Alpha directory and move the code into the depsdev directory itself.
Then, to still allow to have both version supported, just declare 2 constructors. Something like

type API struct {
	client *client.Client
}

// NewV3AlphaAPI creates and returns a V3Alpha API object.
func NewV3AlphaAPI() *API {
	return &API{
		client: client.New(V3AlphaBasePath),
	}
}

// NewV3API creates and returns a V3 API object.
func NewV3API() *API {
	return &API{
		client: client.New(V3BasePath),
	}
}

This would allow you to still be able to instantiate a client for any version of the API you want to reach, but without all the duplicate code.
Regarding the imports, now that all the code lives in the depsdev directory, we can keep the package depsdev like it was, since now it would match the directory name also.

I can push this proposal in this PR, so you can have a look on how it would feel.

@edoardottt
Copy link
Owner

for now there is no difference, but the plan is to implement some methods only for the alpha version, check the issues. eventually they will be added to the stable version too (V3) but for now they aren't available.
also, the Google team said that these should be maintained as two separate packages (google/deps.dev#76).

@zaibon
Copy link
Contributor Author

zaibon commented May 22, 2024

Ok, these things are bound to diverse at some point, that make sense. So let's forget this PR altogether, it doesn't really solve anything then.

@zaibon zaibon closed this May 22, 2024
@edoardottt
Copy link
Owner

fine @zaibon :) If you want to contribute...every contribution is more than welcome!

@zaibon zaibon deleted the rename-package branch May 28, 2024 16:10
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.

2 participants