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

Refactor statistics script for importability #6848

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

queengooborg
Copy link
Contributor

This PR refactors the statistics script to allow it to be imported as a module as well, moving the stats collection into a getStats() function (which is exported) and making printStats() just a console output function. This will simplify the development of a little GitHub Actions bot @foolip will be working on.

@github-actions github-actions bot added the scripts Issues or pull requests regarding the scripts in scripts/. label Oct 6, 2020
@queengooborg queengooborg requested a review from ddbeck October 6, 2020 09:07
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

This will simplify the development of a little GitHub Actions bot @foolip will be working on.

Can I get some elaboration on how the stats are going to be used? If this is being consumed elsewhere, it would be interesting to know the use case in more detail. In the past, we've made changes like this to decompose BCD's own tools for use internal to BCD. This is a bit different—it might even merit exposing this is a proper external API, if it's being consumed externally.

When it comes to the actual changes, I'm finding this monolithic commit difficult to follow.

Explain it to me like it's late on a Friday: why does this do more than export the stats object? Why does this rename printTable to printStats? Why does the processData signature change? A lot happens in one commit, so I need your help to understand the story behind each of the changes you've made here.

@queengooborg
Copy link
Contributor Author

Sure thing! Sorry, I'm not the greatest at explaining changes, haha... 😅 Okay, let's big this a shot:

This PR refactors the script to allow for importability into other code. First, it moves all statistics generation into a getStats function, which returns a stats object based upon the folder and browsers specified. The getStats function is exposed as the module export, allowing other scripts to obtain the raw JSON data. All of the console logging to display the stats is moved into printStats (combining it and printTable), which takes the statistics as an argument (rather than a global variable) generated by getStats.

Does that help?

@ddbeck
Copy link
Collaborator

ddbeck commented Oct 15, 2020

Does that help?

Yes, it does. Thank you!

(At least in my experience, I get a lot out of recording lots of little commits along the way—it's like a logbook so I can remember how to write the description. It's a lifesaver when I have to switch between tasks. 😄)

Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you, @vinyldarkscratch.

Also, just to make sure we didn't introduce anything unintended, I checked out this branch in a worktree, merged in master, then ran the branch stats script and master script, and diffed the two outputs. They both do the same thing. 👍

@ddbeck ddbeck merged commit 5bb65b5 into mdn:master Oct 15, 2020
@queengooborg queengooborg deleted the scripts/statistics branch October 15, 2020 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scripts Issues or pull requests regarding the scripts in scripts/.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants