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 constants to the class and replace static text with constants #74

Merged
merged 1 commit into from
Apr 22, 2016

Conversation

bocharsky-bw
Copy link
Contributor

No description provided.

const LEVEL_CRITICAL = 'critical';
const LEVEL_WARNING = 'warning';
const LEVEL_INFO = 'info';

Copy link
Contributor

Choose a reason for hiding this comment

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

My first thought about this was "It ought to be an enum", but PHP doesn't have those.

Could you instead use an abstract class with those const values instead? (per: http:https://stackoverflow.com/a/254543/456188).

Something like:

abstract class Level {
  const LEVEL_ERROR = 'error';
  const LEVEL_CRITICAL = 'critical';
  const LEVEL_WARNING = 'warning';
  const LEVEL_INFO = 'info';
}

@Crisfole
Copy link
Contributor

Sorry for the long wait for a response!

If you can fix my comments and get the tests passing I'd love to get this merged! (The comments should actually resolve the test issues).

PS: you can run the unit tests locally with phpunit

@bocharsky-bw
Copy link
Contributor Author

@Crisfole your comments make sense! I've updated this PR

@Crisfole
Copy link
Contributor

Looks fantastic! Waiting on the tests, if/when they pass I'd say this is a go.

@brianr eyes?

@brianr
Copy link
Member

brianr commented Apr 15, 2016

LGTM

@Crisfole
Copy link
Contributor

Merging later today along with a few others and a minor version bump.

@Crisfole Crisfole merged commit 10099b0 into rollbar:master Apr 22, 2016
@bocharsky-bw bocharsky-bw deleted the patch-1 branch April 22, 2016 19:02
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.

None yet

3 participants