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

Allow presets to define metadata #76

Merged
merged 2 commits into from
Oct 7, 2019
Merged

Allow presets to define metadata #76

merged 2 commits into from
Oct 7, 2019

Conversation

DanielSanudo
Copy link
Contributor

Summary

Allow presets to define metadata since they can't hook into lifecycles.

Changelog

Entry added.

Test Plan

Tests added.

Copy link
Member

@Swaagie Swaagie left a comment

Choose a reason for hiding this comment

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

Two small comments.

@@ -27,7 +27,12 @@ const mockPluginInfo = {

const mockPresetInfo = {
name: '@gasket/mock-preset',
module: {},
metqadataKey: 'oldMetadataValue',
Copy link
Member

Choose a reason for hiding this comment

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

should this be metqadataKey

Suggested change
metqadataKey: 'oldMetadataValue',
metadataKey: 'oldMetadataValue',

@@ -125,5 +130,9 @@ describe('Metadata plugin', function () {
assume(gasket.metadata.plugins[0]).property('modified', true);
});

it('loads preset metadata and overrides it if collision found', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Could we extend this testcase to both assert the before and after? Based on the typo above, we don't really know whether data changed or not.

@kinetifex kinetifex merged commit 1e48f2e into master Oct 7, 2019
@kinetifex kinetifex deleted the presets-metadata branch October 7, 2019 16:29
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