vue icon indicating copy to clipboard operation
vue copied to clipboard

refactor: Add isString

Open 243083df opened this issue 6 years ago • 3 comments

What kind of change does this PR introduce? (check at least one)

  • [ ] Bugfix
  • [ ] Feature
  • [ ] Code style update
  • [x] Refactor
  • [ ] Build-related changes
  • [ ] Other, please describe:

Does this PR introduce a breaking change? (check one)

  • [ ] Yes
  • [x] No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • [x] It's submitted to the dev branch for v2.x (or to a previous version branch), not the master branch
  • [x] When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • [x] All tests are passing: https://github.com/vuejs/vue/blob/dev/.github/CONTRIBUTING.md#development-setup
  • [x] New/updated tests are included

243083df avatar Apr 16 '18 20:04 243083df

Is this really needed or something just more like over-engineering?

dsonet avatar Apr 18 '18 11:04 dsonet

For me it's good approach to create isString like methods. It increases readability of code.

There are methods like isTrue, isFalse, isObject in shared/util, so why don't create similar for isString ?

Please also take a look here: https://github.com/vuejs/vue/blob/f7b5c86ef2620c1d7ec275f86ae0f380ccaea4ef/src/platforms/web/util/class.js#L37-L39 Mixing different levels of abstraction (isObject near typeof value === 'string') is not a good practice.

lusarz avatar Apr 22 '18 07:04 lusarz

As said lusarz, there is methods like isTrue|isFalse|isObject|isPrimitive, so i dont see over-engineering here

243083df avatar May 08 '18 07:05 243083df