search

vesoft-inc / nebula-python

Client API of Nebula Graph in Python
194 stars 76 forks source link

hotfix invoking the Node tags method #317

Closed haoxins closed 7 months ago

haoxins commented 8 months ago

Sorry for introducing this in #302

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

haoxins commented 8 months ago

BTW, it seems that we don't use @property annotation in this package? I prefer the way of using @property.

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (91afe1b) 77.83% compared to head (e0e974a) 77.84%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #317 +/- ## ======================================= Coverage 77.83% 77.84% ======================================= Files 18 18 Lines 2423 2424 +1 ======================================= + Hits 1886 1887 +1 Misses 537 537 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wey-gu commented 8 months ago

BTW, it seems that we don't use @property annotation in this package?

I prefer the way of using @property.

Yes, this project was not initiated by pythonista, let's polish it together, feel free to drop PR on doing so!

haoxins commented 8 months ago

BTW, it seems that we don't use @property annotation in this package? I prefer the way of using @property.

Yes, this project was not initiated by pythonista, let's polish it together, feel free to drop PR on doing so!

done

wey-gu commented 8 months ago

Sorry, after revisiting for a while, I think it's better not change .tags from a function into a @property.

My suggestions would be:

What do you think @haoxins , please?

haoxins commented 8 months ago

Sorry, after revisiting for a while, I think it's better not change .tags from a function into a @property.

  • It's an API break change
  • Relationship.edge_name and a bunch of other functions are in similar situations, we should either change or left unchanged to maintain consistency of the design

My suggestions would be:

  • leave those getter functions function(without @property)
  • optionally introduce _foo @Property like Node._tags, Relationship._edge_name etc.
  • Use @property for getters in next major version(that such break change is accepted) and to avoid breaking our user code.

What do you think @haoxins , please?

I had the same concerns before I pushed the commit.

I will just revert the commit for this PR

I want to do more naming changes in the future, will give a list in a Github issue.

haoxins commented 8 months ago

Use @property for getters in next major version(that such break change is accepted) and to avoid breaking our user code.

Yes, this project was not initiated by pythonista, let's polish it together, feel free to drop PR on doing so!

I think we can start a Github issue to track the changes that make this package more Pythonic.

wey-gu commented 8 months ago

Use @Property for getters in next major version(that such break change is accepted) and to avoid breaking our user code.

Yes, this project was not initiated by pythonista, let's polish it together, feel free to drop PR on doing so!

I think we can start a Github issue to track the changes that make this package more Pythonic.

Yes, let's do it!!!

wey-gu commented 8 months ago

Sorry, after revisiting for a while, I think it's better not change .tags from a function into a @property.

  • It's an API break change
  • Relationship.edge_name and a bunch of other functions are in similar situations, we should either change or left unchanged to maintain consistency of the design

My suggestions would be:

  • leave those getter functions function(without @property)
  • optionally introduce _foo @Property like Node._tags, Relationship._edge_name etc.
  • Use @property for getters in next major version(that such break change is accepted) and to avoid breaking our user code.

What do you think @haoxins , please?

I had the same concerns before I pushed the commit.

I will just revert the commit for this PR

I want to do more naming changes in the future, will give a list in a Github issue.

Please drop the main list issue as RFC ♥️.

haoxins commented 8 months ago

cc @Nicole00

haoxins commented 8 months ago

@Nicole00 take a look?