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

Evaluate extension point on track config pre-process snapshot #3653

Merged
merged 1 commit into from
May 18, 2023

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Apr 18, 2023

This could possibly be a method for plugins to set 'default' values in track configs xref #3609

The extension point is named Core-preProcessTrackSnapshot and plugins could addToExtensionPoint('Core-preProcessTrackSnapshot',snap=>/*manipulate snap;return snap*/)

I considered adding this to other config snapshot pre-processors but not all of them have a pluginManager available (e.g. the display one does not)

Could adjust to make that available, or accept that only track level is available?

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Apr 18, 2023
@cmdcolin cmdcolin added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Apr 18, 2023
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #3653 (4c8305d) into main (c155f5c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4c8305d differs from pull request most recent head 1a824ab. Consider uploading reports for the commit 1a824ab to get more accurate results

@@           Coverage Diff           @@
##             main    #3653   +/-   ##
=======================================
  Coverage   63.06%   63.07%           
=======================================
  Files         877      877           
  Lines       30167    30167           
  Branches     7272     7272           
=======================================
+ Hits        19026    19028    +2     
+ Misses      10956    10954    -2     
  Partials      185      185           
Impacted Files Coverage Δ
...re/pluggableElementTypes/models/baseTrackConfig.ts 58.33% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin cmdcolin force-pushed the experimental_plugin_addconf branch from 8f854d9 to 4c8305d Compare April 18, 2023 20:16
@rbuels
Copy link
Contributor

rbuels commented Apr 21, 2023

I'd recommend adjusting things to make the pluginManager available where possible. We want to make a lot of extension points that people can manipulate in a lot of different places.

@cmdcolin
Copy link
Collaborator Author

@rbuels it would possibly be somewhat of a 'breaking change' in some cases if we added a pluginManager param to the config schemas like e.g. the baseLinearDisplayConfigSchema, since they are re-exported from the package for use by plugins. could either keep those as is or just accept a breaking change...

@rbuels
Copy link
Contributor

rbuels commented Apr 24, 2023

I seem to vaguely remember we make the pluginManager available in the MST environment, can snapshot preprocessors get it from that?

@cmdcolin
Copy link
Collaborator Author

getEnv does not work in preProcessSnapshot, there is no "self" to getEnv on as far as i could tell

@cmdcolin cmdcolin force-pushed the experimental_plugin_addconf branch from 4c8305d to 1a824ab Compare May 18, 2023 18:11
@cmdcolin cmdcolin force-pushed the experimental_plugin_addconf branch from 1a824ab to e7811e0 Compare May 18, 2023 18:13
@cmdcolin cmdcolin merged commit 001f2cc into main May 18, 2023
@cmdcolin cmdcolin deleted the experimental_plugin_addconf branch May 18, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants