-
Notifications
You must be signed in to change notification settings - Fork 75
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(taps,targets): Support the x-singer.decimal
JSON Schema extension
#2786
Conversation
…the tap configuration
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2786 +/- ##
==========================================
+ Coverage 91.35% 91.37% +0.02%
==========================================
Files 63 63
Lines 5214 5227 +13
Branches 675 676 +1
==========================================
+ Hits 4763 4776 +13
Misses 319 319
Partials 132 132 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #2786 will not alter performanceComparing Summary
|
…the tap configuration
de53f85
to
42a702d
Compare
That's awesome, thanks once again @edgarrmondragon 🙌 |
ff67a40
to
8e95d23
Compare
…to edgarrmondragon/feat/singer-decimal
e27b663
to
9df828b
Compare
…to edgarrmondragon/feat/singer-decimal
584d3b5
to
6ec48ae
Compare
…to edgarrmondragon/feat/singer-decimal
Reviewer's Guide by SourceryThis pull request introduces support for the Class diagram for SingerDecimalTypeclassDiagram
class SingerDecimalType {
+string_format = "singer.decimal"
}
SingerDecimalType --|> StringType
class StringType
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @edgarrmondragon - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a default value for
max_varchar_length
in theJSONSchemaToSQL
constructor to avoid breaking changes. - It might be worth adding a test case that checks the precision and scale are passed through correctly to the SQLAlchemy
DECIMAL
type.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
singer.decimal
JSON Schema extensionx-singer.decimal
JSON Schema extension
@sourcery-ai review |
TODO:
Related:
📚 Documentation preview 📚: https://meltano-sdk--2786.org.readthedocs.build/en/2786/
Summary by Sourcery
Adds support for the
singer.decimal
JSON Schema extension. This allows numbers to be represented as strings with thesinger.decimal
format instead of as numbers. A newuse_singer_decimal
option is added to theSQLToJSONSchema
class to control whether numbers are represented asstring
with thesinger.decimal
format or asnumber
.New Features:
singer.decimal
JSON Schema extension, allowing numbers to be represented as strings with thesinger.decimal
format instead of as numbers.Enhancements:
use_singer_decimal
option to theSQLToJSONSchema
class to control whether numbers are represented asstring
with thesinger.decimal
format or asnumber
.Tests:
singer.decimal
format.singer.decimal
types to SQLAlchemy DECIMAL types.