casbin / xorm-adapter

Xorm adapter for Casbin
https://github.com/casbin/casbin
Apache License 2.0
384 stars 58 forks source link

Add custom tablename support #30

Closed cyjaysong closed 4 years ago

cyjaysong commented 4 years ago

Add custom tablename support

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-8.03%) to 58.803% when pulling da0b7c443d42fce3e570c0a1582c3ac44d6ce9db on CyJaySong:master into 695d84de0d262c204a583e4a3fb1a59e04fd0b37 on casbin:master.

hsluoyz commented 4 years ago

@nodece @GopherJ @dovics @divy9881 please review.

nodece commented 4 years ago

@CyJaySong please add the NewAdapterWithTableName method.

nodece commented 4 years ago

When you call the NewAdapter, casbin table has been created.

dovics commented 4 years ago

I don't like to use Adapter as the field of casbinRule, It looks really strange. Perhaps a global variable would be better, what do you think @CyJaySong @hsluoyz @nodece

nodece commented 4 years ago

ok great, @CyJaySong please add a global method named is SetTableName for set table name.

hsluoyz commented 4 years ago

I don't like to use Adapter as the field of casbinRule, It looks really strange. Perhaps a global variable would be better, what do you think @CyJaySong @hsluoyz @nodece

I agree. It's really weird to have adapter there.

hsluoyz commented 4 years ago

@CyJaySong can you make changes based on review comments?

cyjaysong commented 4 years ago

I was negligent. I will modify it

Zixuan Liu notifications@github.com 于2020年7月9日周四 下午10:35写道:

When you call the NewAdapter, casbin table has been created.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/casbin/xorm-adapter/pull/30#issuecomment-656165472, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHAB2LY3MAC5VZK2MFYAZOTR2XIUTANCNFSM4OVNXKMQ .

cyjaysong commented 4 years ago

It is not appropriate to use global variables。There is a problem using more than one xorm adapter

dovics notifications@github.com 于2020年7月9日周四 下午10:53写道:

I don't like to use Adapter as the field of casbinRule, It looks really strange. Perhaps a global variable would be better, what do you think @CyJaySong https://github.com/CyJaySong @hsluoyz https://github.com/hsluoyz @nodece https://github.com/nodece

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/casbin/xorm-adapter/pull/30#issuecomment-656175971, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHAB2L44K7MN32IONPZATADR2XKYHANCNFSM4OVNXKMQ .

nodece commented 4 years ago

@CyJaySong Could you add tests for the feature?

cyjaysong commented 4 years ago

@CyJaySong Could you add tests for the feature?

Is there anything wrong with that?

nodece commented 4 years ago

Looks good, but we also need to write test case to test the feature.

cyjaysong commented 4 years ago

Looks good, but we also need to write test case to test the feature.

I have no inspiration

hsluoyz commented 4 years ago

tableName should not be put into Casbin rule. I think the code quality of this PR is not ideal. Sorry that I have to close it. Feel free to raise a new good one.

nodece commented 4 years ago

Hi @hsluoyz , The tableName will not be added to DB, due to we add xorm: "-" to struct tag, so the implementation is correct.

We can't define a global variable for set table name.

It is not appropriate to use global variables。There is a problem using more than one xorm adapter

hsluoyz commented 4 years ago

OK. Go ahead!

cyjaysong commented 4 years ago

OK. Go ahead!

So, do you agree to merge 😅

hsluoyz commented 4 years ago

OK. Go ahead!

So, do you agree to merge 😅

@nodece can decide.

nodece commented 4 years ago

@hsluoyz Could you give me the repository permission?

hsluoyz commented 4 years ago

OK. Granted.