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

feat(v-3): QGIS essentials #369

Open
wants to merge 4 commits into
base: v3-dev
Choose a base branch
from
Open

Conversation

dogukankaratas
Copy link

adds QGIS essentials.

Geometry:

  • Polyline
  • Mesh

Interface:

  • IHasArea
  • IHasVolume

Proxies:

  • InstanceProxy
  • InstanceDefinitionProxy
  • ColorProxy
  • GroupProxy

@dogukankaratas dogukankaratas changed the base branch from main to v3-dev December 13, 2024 14:45
Copy link
Contributor

@KatKatKateryna KatKatKateryna left a comment

Choose a reason for hiding this comment

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

I went a bit too detailed, but hopefully we won't need to touch these again!

Some other comments not related to this specific commit, but related to the new objects:

  • Point class "distance_to" ignores Point units (extra test can reveal it). Also a problem in C#
  • Line method "from_coords" doesn't exist in C#

src/specklepy/objects/geometry.py Outdated Show resolved Hide resolved
src/specklepy/objects/geometry.py Outdated Show resolved Hide resolved
return result

@classmethod
def from_list(cls, coords: List[float], units: str | Units) -> "Polyline":
Copy link
Contributor

Choose a reason for hiding this comment

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

if we follow C# methods, should we assume units are passed as the last element in the list (without keyword)? Same for Line

Copy link
Author

Choose a reason for hiding this comment

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

the keywords are required for the class definition but not in the classmethods. So, for classmethods, yes.

Copy link
Author

Choose a reason for hiding this comment

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

I decided to keep units as a seperate argument to be able to compatible with all the classes.

src/specklepy/objects/geometry.py Outdated Show resolved Hide resolved
src/specklepy/objects/tests/mesh_test.py Outdated Show resolved Hide resolved
src/specklepy/objects/proxies.py Outdated Show resolved Hide resolved
src/specklepy/objects/proxies.py Outdated Show resolved Hide resolved
src/specklepy/objects/proxies.py Outdated Show resolved Hide resolved
src/specklepy/objects/proxies.py Outdated Show resolved Hide resolved
src/specklepy/objects/proxies.py Outdated Show resolved Hide resolved
@KatKatKateryna
Copy link
Contributor

Formatting PR (you can ignore it and format yourself after changes, if you prefer): #370

@dogukankaratas
Copy link
Author

I went a bit too detailed, but hopefully we won't need to touch these again!

Some other comments not related to this specific commit, but related to the new objects:

  • Point class "distance_to" ignores Point units (extra test can reveal it). Also a problem in C#
  • Line method "from_coords" doesn't exist in C#

Fixed.

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