Skip to content

Remove non-static Create methods#230

Merged
nwithan8 merged 4 commits intomasterfrom
vb_support
Mar 14, 2022
Merged

Remove non-static Create methods#230
nwithan8 merged 4 commits intomasterfrom
vb_support

Conversation

@nwithan8
Copy link
Copy Markdown
Contributor

@nwithan8 nwithan8 commented Mar 10, 2022

Per #151 , Visual Basic cannot tell the difference between a static and non-static version of the same method signature. This is an issue because we have both static and non-static Create methods in a handful of our classes.

The non-static methods, however, are for creating/recreating an object in case its id is blank, which logically does not make much sense. Why are we checking, i.e. when calling myOrder.Buy() whether myOrder exists? Of course it does.

The methods were likely implemented for a single-altered structure, accounting for if the id was somehow blank/not returned by the API. There are better ways to handle this. For now, I have removed the id check, but if it is truly necessary, we should have an exception thrown, rather have the library attempt to regenerate the object. In that case, we can rollback part of these changes and add a new ResourceNotCreated expection.

By removing these non-static Create methods, there should be no more ambiguity for Visual Basic.

Comment thread EasyPost.Net/Address.cs
…n C# code can compile correctly for other language environments

- Add VB and FSharp compile tests to GitHub CI
@nwithan8
Copy link
Copy Markdown
Contributor Author

@Justintime50 per our discussions, I have added an extra test for the Visual Basic test suite. There's no need to recreate the entire test suite for the F# and Visual Basic variants.

There are two tests in the Visual Basic test suite:

  • A basic test that is expected to throw a ClientNotConfigured error since there is no API key set. As long as it throws the expected error, and nothing else, we can assume Visual Basic is able to import our C# code properly.
  • A basic test to create an Address. Again, this will produce the ClientNotConfigured error, but as long as no other errors are thrown, we can consider it a success. Address creation was one of a few spots where the competing named functions was causing compile errors. As long as this unit test pass, we can assume that issue has been resolved.

In fact, the test suite actually running is largely unnecessary. Just by the nature of the project (featuring formerly-problematic code) compiling, we can confirm that our C# code can be imported successfully into a Visual Basic or F# environment.

…I key, will produce a ClientNotConfigured error. But as long as it doesn't produce any other errors, functionality is working as expected.
Copy link
Copy Markdown
Member

@Justintime50 Justintime50 left a comment

Choose a reason for hiding this comment

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

Nice work!

@jchen293 jchen293 marked this pull request as ready for review March 14, 2022 18:13
@nwithan8 nwithan8 merged commit a0006c0 into master Mar 14, 2022
@nwithan8 nwithan8 deleted the vb_support branch March 14, 2022 19:50
@nwithan8 nwithan8 changed the title WIP: Removing non-static Create methods Remove non-static Create methods Mar 14, 2022
@nwithan8 nwithan8 mentioned this pull request Apr 19, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants