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 uppercase option #7

Merged
merged 6 commits into from
Jul 10, 2017
Merged

Add uppercase option #7

merged 6 commits into from
Jul 10, 2017

Conversation

101100
Copy link
Contributor

@101100 101100 commented Jun 27, 2017

This adds options to the GUID inserter:

  • insertGuid.showLowercase
  • insertGuid.showUppercase
  • insertGuid.showCodeSnippets

By default, they are set with lowercase and code snippets on to match the old behaviour. If they are all set to false, then just the lowercase options are shown.

Closes #6.

Copy link
Owner

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Overall looks good. I like the implementation, but make sure you keep the current behavior with all new options off, and the regression tests to go with them.

test('quick pick 3 is C structure', () => {
var g = Guid.parse('12341234-1234-1234-1234-123412341234');
var items = GuidCommands.getQuickPickItems(g);
test('quick pick iterms are correct with only uppercase enabled', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

Typo: "iterms"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed (in all spots).

});

test('quick pick 2 is registry string', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

The original tests should be retained as regression tests. The overall change looks good, but if people have muscle memory for the current way, that should be retained. If they change the options you've added, I think it's fine to change those. But the defaults should still work as they did before and retaining the old tests as regression tests (pass whatever parameters to getQuickPickItems you need to, or use default parameter values) will help ensure that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to do a full test on this. I set the defaults here to match the old behaviour, but I don't know how to create an integration test to show that the defaults have the correct behaviour.

@heaths
Copy link
Owner

heaths commented Jul 6, 2017

@101100 I'd love to merge this with the couple of requested changes done. Overall it looks good. Changes are just about retaining existing behavior by default. Setting new options can change that since they themselves new.

@101100
Copy link
Contributor Author

101100 commented Jul 6, 2017

@heaths Do you know of any examples that I could base tests off of for preference defaults?

Otherwise, I've fixed the typo.

@heaths
Copy link
Owner

heaths commented Jul 6, 2017

I'd just leave the existing tests as-is when the options you added are false (as they would be by default), and then add the tests you already wrote as all-new tests where you test the combinations of options.

@101100
Copy link
Contributor Author

101100 commented Jul 7, 2017

I've re-added the previous tests. The options passed in are true, false, true because that is what they are set to by default in package.json.

@heaths heaths merged commit 86da2b2 into heaths:master Jul 10, 2017
@101100 101100 deleted the add-uppercase-option branch July 10, 2017 20:10
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