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

Add defaultSizeKey #129

Merged

Conversation

OliverJAsh
Copy link
Collaborator

Problem

Re. #14 (comment)

Solution

Explain your approach. Sometimes it helps to justify your approach against some others that you didn't choose to explain why yours is better.

TODO

  • 🤓 Add & update tests (always try to increase test coverage)
  • 🔬 Ensure CI is passing (yarn lint:ci, yarn test, yarn tsc:ci)
  • 📖 Update relevant documentation

Copy link
Owner

@paularmstrong paularmstrong left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

expect(state.sizeKey).toBe('stat');
});

test('resets the sizeKey to the first in list if both the current and the default are not in the new set', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

💯

@paularmstrong paularmstrong merged commit b621ac7 into paularmstrong:next Feb 18, 2020
@OliverJAsh OliverJAsh deleted the oja/feature/default-size-key branch February 18, 2020 17:06
@OliverJAsh
Copy link
Collaborator Author

@paularmstrong I've just realised that the Comparator should also honour this. Currently the default is hardcoded to gzip:

sizeKey = 'gzip'

If that makes sense to you I can try to raise another PR for that.

@paularmstrong
Copy link
Owner

Whoops! Yeah that sounds good. Maybe time to also think about making all the configuration a little more automatic with the way it spreads through the various parts of Build Tracker.

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