-
Notifications
You must be signed in to change notification settings - Fork 24
feat: update the manifest model to include the setting for work objects #247
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
Conversation
e5feb56 to
de46401
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
==========================================
- Coverage 63.02% 62.99% -0.04%
==========================================
Files 212 212
Lines 21857 21857
==========================================
- Hits 13776 13769 -7
- Misses 7015 7021 +6
- Partials 1066 1067 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
de46401 to
af994ce
Compare
zimeg
left a comment
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.
@vegeris LGTM! I find these values appear in app settings after installing or updating a manifest 🤩
No comments requesting change but I did add a changelog section to this PR for upcoming release notes - please of course make edits to what makes more sense! 📚
Otherwise, let's get this merged and perhaps released soon?
| } | ||
|
|
||
| type RichPreviews struct { | ||
| EntityTypes []string `json:"entity_types,omitempty" yaml:"entity_types,flow,omitempty"` |
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.
📝 though: The omitempty attribute worries me with slices since it's not obvious how to keep an empty list of entity types, but I notice the API errors in that case so no worries!
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.
Yea, this is the common struggle of matching the API defaults with Golang language rules. Seems like we'll want to keep the omitempty to align with the API.
| RichPreviews: &RichPreviews{ | ||
| EntityTypes: []string{"slack#/entities/file"}, | ||
| }, |
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.
📠 praise: Thanks for including test, as always!
This reverts commit e9d48b7.
|
|
||
| type RichPreviews struct { | ||
| EntityTypes []string `json:"entity_types,omitempty" yaml:"entity_types,flow,omitempty"` | ||
| IsActive bool `json:"is_active,omitempty" yaml:"is_active,flow,omitempty"` |
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.
I was on the fence about including this property but after some discussion, we might want to support the dev setting this rather than needing to remove the entire rich_previews setting to disable (so I added it back)
mwbrooks
left a comment
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.
✅ Looks good to me, thanks for adding support for work objects!
📝 I believe this would be a semver:minor because we're adding a new feature (work object support).
Changelog
The app manifest values for
entity_typesare now gathered when reading an app manifest.Summary
This PR updates the app manifest model to include work object settings.
Test
Requirements