Skip to content

Use constants instead of magic strings for trigger names. #670

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 1 commit into from
Feb 26, 2016

Conversation

nlutsenko
Copy link
Contributor

In all of these occurences - we are using a magic string (stirng literal created in place) instead of a constant - just use the constant, since we have triggers.js imported.
There are actually no more occurences of this, so we might as well continue using constants for these in future at all times.

@drew-gross
Copy link
Contributor

I wish JS had real enums :( I find that in practise, this strings-as-fake-enums technique doesn't really end up solving any problems in javascript.

@drew-gross drew-gross assigned nlutsenko and unassigned drew-gross Feb 26, 2016
@nlutsenko
Copy link
Contributor Author

Would be amazing if JS only had those!
The problem we are trying to solve here is actually consistency - since we are using a type, if it's missing - it will be undefined, which would at least surface things better, also provides a much better ability to search and lexer understanding of our stuff.

nlutsenko added a commit that referenced this pull request Feb 26, 2016
Use constants instead of magic strings for trigger names.
@nlutsenko nlutsenko merged commit 3b1165b into master Feb 26, 2016
@nlutsenko nlutsenko deleted the nlutsenko.strings branch February 26, 2016 04:45
@flovilmart
Copy link
Contributor

JS has enums, I believe I saw that in douglas crawford

@drew-gross
Copy link
Contributor

They still aren't real enums. The newest version of TypeScript adds real enums, or at least as real as you get in JS land.

@flovilmart
Copy link
Contributor

Should we Object.freeze the Types?

@drew-gross
Copy link
Contributor

surewhynot

@flovilmart
Copy link
Contributor

I though about refactoring a bit the triggers also as it's all over the place now with the hooks controller...

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.

4 participants