Skip to content

reduce plot api imports #3819

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

Merged
merged 2 commits into from
May 8, 2019
Merged

reduce plot api imports #3819

merged 2 commits into from
May 8, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Apr 29, 2019

Refactoring plot_api files (i.e. excluding plot_api.js) to reduce imported functions and help increase code readability.

@etpinard

@etpinard
Copy link
Contributor

etpinard commented May 2, 2019

To be honest @archmoj, I don't see how this makes our code more readable.

I agree our import/export statements could be standardised better, but I think we can wait when we'll make a push for ES6 to spend time on this.

@etpinard
Copy link
Contributor

etpinard commented May 6, 2019

@archmoj ok to close this thing?

@archmoj
Copy link
Contributor Author

archmoj commented May 6, 2019

To be honest @archmoj, I don't see how this makes our code more readable.

I agree our import/export statements could be standardised better, but I think we can wait when we'll make a push for ES6 to spend time on this.

Your call.
Anyway here is one example if one searches for isPlainObject in https://github.com/plotly/plotly.js/blob/b6503ac2682bab35e7e6949696a7ce029cba00da/src/plot_api/plot_schema.js there are two ways which is called i.e. with/without Lib..

@etpinard
Copy link
Contributor

etpinard commented May 6, 2019

Ok, I agree the patch you made to plot_schema.js is 👌

var Lib = require('../lib');
var Plots = require('../plots/plots');
var AxisIds = require('../plots/cartesian/axis_ids');
var subplotsRegistry = require('../plots/plots').subplotsRegistry;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only function used from Plots in this file is subplotsRegistry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to enforce this rule using eslint? If not, than we don't need to do this.

@etpinard
Copy link
Contributor

etpinard commented May 8, 2019

Thanks @archmoj 💃

@archmoj archmoj merged commit 8cdf799 into master May 8, 2019
@archmoj archmoj deleted the reduce-plot-api-imports branch May 8, 2019 14:39
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.

2 participants