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

add user define database #43

Merged
merged 3 commits into from
Sep 4, 2020
Merged

add user define database #43

merged 3 commits into from
Sep 4, 2020

Conversation

00LT00
Copy link
Member

@00LT00 00LT00 commented Aug 3, 2020

Signed-off-by: 00LT00 lightning@hdu.edu.cn

Fix: #40

00LT00 added 2 commits August 4, 2020 02:10
Signed-off-by: 00LT00 <lightning@hdu.edu.cn>
Signed-off-by: 00LT00 <lightning@hdu.edu.cn>
@coveralls
Copy link

coveralls commented Aug 3, 2020

Coverage Status

Coverage decreased (-2.9%) to 59.836% when pulling 6b75244 on 00LT00:master into 81c2dc6 on casbin:master.

@hsluoyz
Copy link
Member

hsluoyz commented Aug 4, 2020

@rico-ci please review.

Copy link

@rico-ci rico-ci left a comment

Choose a reason for hiding this comment

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

I generally find that the positional checking of the type of a specific parameter passed in params will eventually turn out to be very fragile. I am not aware of any better patterns in Go specifically. I suppose for now it will do.
On another note, I am wondering if there are cases where only a subset of the three arguments are passed (tableName, databaseName, dbSpecified)?

}else if len(params) == 1 {
switch p1:=params[0].(type) {
case bool:
a.dbSpecified = p1
Copy link

Choose a reason for hiding this comment

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

How can one set dbSpecified to True without providing the parameters required for that db?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rico-ci If I understand correctly, you can just pass in a bool variable as the value of dbSpecified.

Copy link

Choose a reason for hiding this comment

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

When would you encounter the case that dbSpecified is set but not the databaseName or the tableName?

Copy link
Member Author

Choose a reason for hiding this comment

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

When there is no databseName or tableName on the old version, you can set dbspecified separately. So if you dont set dbname and tablename, it will use the default values.
Default values are set in adapter.go#L94-L96

Copy link
Member Author

Choose a reason for hiding this comment

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

On the other hand, it is also compatible with older versions

Copy link
Member Author

Choose a reason for hiding this comment

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

@rico-ci Do you have any question?

adapter.go Show resolved Hide resolved
adapter.go Outdated Show resolved Hide resolved
adapter.go Outdated Show resolved Hide resolved
adapter.go Outdated Show resolved Hide resolved
@hsluoyz
Copy link
Member

hsluoyz commented Aug 10, 2020

@00LT00 please fix.

@rico-ci
Copy link

rico-ci commented Aug 11, 2020

Once my comments are addressed by @00LT00 I'm happy for this to be merged. Will there be any incompatibilities with casbin-server? Will we have to adapt code there and in other projects using this?

@00LT00
Copy link
Member Author

00LT00 commented Aug 11, 2020

Once my comments are addressed by @00LT00 I'm happy for this to be merged. Will there be any incompatibilities with casbin-server? Will we have to adapt code there and in other projects using this?

It is just add two optional parameters. And the default value is same as the old versions. So I think it will work as before. If you have any problems, welcome to create issues.

@hsluoyz
Copy link
Member

hsluoyz commented Aug 31, 2020

ping @rico-ci

@nodece
Copy link
Member

nodece commented Aug 31, 2020

@00LT00 could you format the code? I can merge it to gorm-adapter.

Signed-off-by: 00LT00 <lightning@zerokirin.online>
@00LT00
Copy link
Member Author

00LT00 commented Sep 1, 2020

Already formatted code @nodece

@nodece nodece merged commit 9cc32f0 into casbin:master Sep 4, 2020
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.

User should define database
5 participants