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

refactor(objects): removes unused classes and constructors #151

Merged
merged 19 commits into from
Nov 4, 2024

Conversation

clairekuang
Copy link
Member

@clairekuang clairekuang commented Oct 28, 2024

This is a deep clean of our Objects classes, including:

  • removing all unused classes (based on current cs and cpp converters)
  • removing all unused constructors (including grasshopper Schema constructors and flags)
  • adding required keywords and nullability where known
  • flagging props to be deprecated with obsolete and jsonignore
  • adding a LegacyV2 class with deprecation flags for any removed classes that we do not want dynamically traversed (eg blocks, views, parameters).

The target behavior of V2 -> V3 receives after this pr should remain mostly the same as release:

  • all removed classes that exist in V2 should fall back to conversion as Base, and display values are received where they exist
  • no supported classes with deprecated props should fail to deserialize.
  • main difference: removed classes without display value will be dynamically traversed
  • also deprecates model curves, will be sending revit curves as regular curves

See testing results in: https://linear.app/speckle/issue/CNX-713/objects-testing

Copy link

linear bot commented Oct 28, 2024

@clairekuang clairekuang marked this pull request as draft October 28, 2024 18:18
@clairekuang clairekuang changed the title refactor(objects): removes unused classes and gh schema constructors refactor(objects): removes unused classes and constructors Oct 28, 2024
@AlanRynne
Copy link
Member

I'm loving this already ♥️

@clairekuang clairekuang marked this pull request as ready for review October 29, 2024 16:24
/// </summary>
[JsonIgnore, Obsolete("end angle should be calculated from arc endpoint and plane if needed", true)]
public double? endAngle { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Technically, we could remove these, and it would not break the json layer.

@@ -285,21 +145,19 @@ public List<double> ToList()
/// <returns>A new <see cref="Arc"/> with the values assigned from the list.</returns>
public static Arc FromList(List<double> list)
Copy link
Member

Choose a reason for hiding this comment

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

What did we say we'd do here r.e. v2

Copy link
Member Author

Choose a reason for hiding this comment

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

decided to keep the list length the same as in V2 and just pass in dummy values for properties that were marked obsolete

JsonProperty(NullValueHandling = NullValueHandling.Ignore),
Obsolete("Access coordinates using XYZ and weight fields", true)
]
private new List<double> value
Copy link
Member

Choose a reason for hiding this comment

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

should be bring these back?

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 78.98089% with 33 lines in your changes missing coverage. Please review.

Project coverage is 63.93%. Comparing base (fba0c46) to head (ef22e16).
Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
src/Speckle.Objects/Geometry/Arc.cs 16.12% 26 Missing ⚠️
...ckle.Sdk.Serialization.Tests/SerializationTests.cs 50.00% 1 Missing and 2 partials ⚠️
src/Speckle.Objects/Geometry/Box.cs 50.00% 2 Missing ⚠️
src/Speckle.Objects/Geometry/Line.cs 0.00% 1 Missing ⚠️
src/Speckle.Objects/Geometry/Pointcloud.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              dev     #151       +/-   ##
===========================================
+ Coverage   33.26%   63.93%   +30.67%     
===========================================
  Files         378      223      -155     
  Lines       12374     8773     -3601     
  Branches     1061      989       -72     
===========================================
+ Hits         4116     5609     +1493     
+ Misses       8013     2884     -5129     
- Partials      245      280       +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adamhathcock
adamhathcock previously approved these changes Oct 31, 2024
Copy link
Member

@JR-Morgan JR-Morgan left a comment

Choose a reason for hiding this comment

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

Claire is going to patch in a deserialization target for Revit instances, blocks, and maybe a couple others where we're not happy with the change to dynamic traversal.

I've suggested using a single class for this, and use multiple DeprecatedSpeckleTypeAttributes


I can see no risky changes in any of the modified object models, except arc which is known about and acceptable.

Otherwise, everything here is great. Claire and Bilal have been very through with testing.

Copy link
Member

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

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

♥️

@AlanRynne AlanRynne enabled auto-merge (squash) November 4, 2024 17:02
@AlanRynne AlanRynne merged commit 8a148b8 into dev Nov 4, 2024
2 checks passed
@AlanRynne AlanRynne deleted the claire/cnx-687-purge-unused-classes-from-objects branch November 4, 2024 17:05
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.

4 participants