Skip to content

Inverse dependencies link between translation and rendermime interfaces #12493

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.

Sign up for GitHub

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

Jump to bottom
Merged
jtpio merged 2 commits into from
Apr 29, 2022
Merged

Inverse dependencies link between translation and rendermime interfaces #12493

jtpio merged 2 commits into from
Apr 29, 2022

Conversation

@fcollonval
Copy link
Member

@fcollonval fcollonval commented Apr 29, 2022 •
edited
Loading

References

Fixes #11748

Code changes

The rendermime interfaces package should be independent of the other packages.

Remove IRenderer.translator? as a renderer has only two contracts:

  • Be a Widget
  • Have a renderModel method

It receives the translator object at construction if it needs it.

User-facing changes

None

Backwards-incompatible changes

Remove IRenderer.translator?

@jupyterlab-probot
Copy link

jupyterlab-probot bot commented Apr 29, 2022

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@fcollonval fcollonval added the api-change A change that should be accompanied by a major version increase label Apr 29, 2022
@fcollonval fcollonval added this to the 4.0 milestone Apr 29, 2022
jtpio
jtpio reviewed Apr 29, 2022
View reviewed changes
@jtpio
Copy link
Member

jtpio commented Apr 29, 2022

Thanks @fcollonval 👍

Should we also "unbump" the package, so it's back to ^3.x.x?

fcollonval
fcollonval commented Apr 29, 2022
View reviewed changes
packages/rendermime-interfaces/src/index.ts
/**
* The application language translator.
*/
translator?: ITranslator;
Copy link
Member Author

@fcollonval fcollonval Apr 29, 2022

Choose a reason for hiding this comment

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

If this change is fine to left the package at v3.x, I'm definitely 💯 for avoiding downstream mimetype extension to require bumping their package for JLab v4.

I'll raise the question at the next meeting.

Copy link
Member

@jtpio jtpio Apr 29, 2022

Choose a reason for hiding this comment

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

Sounds good.

jtpio
jtpio approved these changes Apr 29, 2022
View reviewed changes
Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit 12c5ed9 into jupyterlab:master Apr 29, 2022
@fcollonval fcollonval deleted the fix/11748-Dont-bump-version-for-jupyterlab/rendermime-interfaces branch April 29, 2022 13:34
@github-actions
Copy link
Contributor

github-actions bot commented Apr 29, 2022

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table
Test file large_code_notebook large_md_notebook
open
chromium
actual 16486 <- [17224 - 17609 - 18013] -> 19674 3110 <- [3324 - 3404 - 3528] -> 4283
expected 16023 <- [16737 - 17060 - 17556] -> 18551 3094 <- [3232 - 3333 - 3429] -> 3628
Mean relative change 3.0% ± 0.9% 3.5% ± 1.4%
switch-from
chromium
actual 672 <- [723 - 748 - 788] -> 998 506 <- [550 - 577 - 601] -> 783
expected 660 <- [721 - 743 - 759] -> 818 488 <- [531 - 550 - 561] -> 655
Mean relative change 4.1% ± 2.2% 8.5% ± 2.6%
switch-to
chromium
actual 395 <- [534 - 560 - 591] -> 711 323 <- [366 - 383 - 399] -> 473
expected 474 <- [523 - 546 - 573] -> 624 302 <- [357 - 367 - 382] -> 434
Mean relative change 2.4% ± 2.2% 4.5% ± 2.0%
close
chromium
actual 1229 <- [1307 - 1349 - 1390] -> 1580 565 <- [605 - 630 - 652] -> 758
expected 1240 <- [1305 - 1339 - 1375] -> 1473 554 <- [592 - 611 - 623] -> 678
Mean relative change 1.1% ± 1.2% 3.7% ± 1.5%

Changes are computed with expected as reference.

@afshin afshin mentioned this pull request May 5, 2022
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jtpio jtpio jtpio approved these changes

Assignees

@fcollonval fcollonval

Labels

api-change A change that should be accompanied by a major version increase documentation maintenance pkg:rendermime pkg:translation

Projects

None yet

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

Don't bump version for @jupyterlab/rendermime-interfaces

2 participants

@fcollonval @jtpio