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

Typescript checking for config slot names #3629

Merged
merged 1 commit into from
Apr 19, 2023
Merged

Conversation

rbuels
Copy link
Contributor

@rbuels rbuels commented Apr 5, 2023

This PR adds type checking of config slot names to the configuration system TS. It can figure out the specific config schema surprisingly often.

Other things in this PR:

  • fixes some type errors found by the new checking
  • changes a lot of imports to import from '@jbrowse/core/configuration' at the top level instead of '/configurationSchema' under it for better forward compatibility. The old import location still works though.

@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 5, 2023
@rbuels rbuels added internal and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Apr 7, 2023
@rbuels rbuels marked this pull request as draft April 10, 2023 16:33
@rbuels rbuels changed the title some config system typescript improvements typescript checking for config slot names Apr 11, 2023
@rbuels rbuels force-pushed the configuration-typescript branch 2 times, most recently from 9207dfb to ed6001d Compare April 11, 2023 16:03
@rbuels rbuels marked this pull request as ready for review April 11, 2023 18:07
* improve readConfObject ts for slot names
* slot name checking support for explicitIdentifier
* move mouseover config slot in linear from LinearBasicDisplay to BaseLinearDisplay config schema
* remove some commented code and typos
* change most things to import config things from @jbrowse/core/configuration top-level
* ts checking of base-conf working
@rbuels rbuels force-pushed the configuration-typescript branch from ed6001d to 9ee6eb9 Compare April 18, 2023 16:38
@rbuels
Copy link
Contributor Author

rbuels commented Apr 18, 2023

rebased and squashed just now onto main. @cmdcolin does this look OK to merge now?

const refNameAliasesAdapterConf = conf.refNameAliases?.adapter
const cytobandAdapterConf = conf.cytobands?.adapter
const sequenceAdapterConf = conf.sequence.adapter
const refNameAliasesAdapterConf = conf?.refNameAliases?.adapter
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason for additional undefined checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, TS found that some of those things can be undefined

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that the configuration is a safeReference so that could be indeed possibly undefined, but my editor says self.configuration is type any so wonder if it regressed back to any somehow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it did regress back, somehow, let me see if I can improve the types there

Copy link
Contributor Author

@rbuels rbuels Apr 18, 2023

Choose a reason for hiding this comment

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

Actually, upon looking into it further, I don't think it did regress, I think I may have been using a different version of TS or mst that caught that error

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you identify which versions? the repo of course has the lock file, and ts/mst versions haven't changed substantially for awhile

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #3629 (9ee6eb9) into main (c155f5c) will increase coverage by 0.00%.
The diff coverage is 82.85%.

@@           Coverage Diff           @@
##             main    #3629   +/-   ##
=======================================
  Coverage   63.06%   63.07%           
=======================================
  Files         877      877           
  Lines       30167    30168    +1     
  Branches     7272     7271    -1     
=======================================
+ Hits        19026    19028    +2     
+ Misses      10956    10955    -1     
  Partials      185      185           
Impacted Files Coverage Δ
packages/core/Plugin.ts 100.00% <ø> (ø)
...ages/core/data_adapters/BaseAdapter/BaseAdapter.ts 72.72% <ø> (ø)
packages/core/pluggableElementTypes/AdapterType.ts 83.33% <ø> (ø)
...kages/core/pluggableElementTypes/ConnectionType.ts 81.81% <ø> (ø)
packages/core/pluggableElementTypes/DisplayType.ts 66.66% <ø> (ø)
.../core/pluggableElementTypes/InternetAccountType.ts 63.63% <ø> (ø)
...ore/pluggableElementTypes/TextSearchAdapterType.ts 85.71% <ø> (ø)
...gableElementTypes/renderers/FeatureRendererType.ts 74.50% <ø> (ø)
...re/pluggableElementTypes/renderers/RendererType.ts 81.25% <ø> (ø)
...eElementTypes/renderers/ServerSideRendererType.tsx 38.09% <ø> (ø)
... and 32 more

... and 2 files with indirect coverage changes

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

@cmdcolin
Copy link
Collaborator

should be ok

@rbuels rbuels merged commit 49b01fd into main Apr 19, 2023
@rbuels rbuels deleted the configuration-typescript branch April 19, 2023 17:15
@cmdcolin cmdcolin changed the title typescript checking for config slot names Typescript checking for config slot names Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants