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

Define constants for field names and statuses #200

Merged
merged 2 commits into from Aug 14, 2015
Merged

Define constants for field names and statuses #200

merged 2 commits into from Aug 14, 2015

Conversation

jml
Copy link
Contributor

@jml jml commented Aug 10, 2015

Rather than using anything nice like Python 3.4's enum or twisted.python.constants, I've just defined a bunch of variables with similar-looking names. I think this makes the code more readable and makes some of the structure of eliot more explicit.

I haven't updated usage in tests, but can if you'd like.

Review on Reviewable

@itamarst
Copy link
Owner

Perhaps these should also be part of the public API?

@jml
Copy link
Contributor Author

jml commented Aug 10, 2015

Happy to do that, perhaps as a separate patch?

@jml
Copy link
Contributor Author

jml commented Aug 12, 2015

Actually, now I think making APIs public should definitely be part of a separate patch. Now that constantly is released I'd like to look at using it. I did a quick experiment with switching the various STATUS fields to be ValueConstant and noticed two things:

  1. SUCCEEDED_STATUS et. al. get renamed to STATUS.SUCCEEDED etc. (which would break a public API)
  2. Serialization starts to fail with exceptions.TypeError: <STATUS=STARTED> is not JSON serializable

@jml jml mentioned this pull request Aug 14, 2015
@itamarst
Copy link
Owner

You'd need to use STATUS.SUCCEEDED.value to prevent the serialization problems.

@itamarst
Copy link
Owner

Thanks, definitely an improvement.

itamarst added a commit that referenced this pull request Aug 14, 2015
Define constants for field names and statuses.
@itamarst itamarst merged commit 5d7fd6c into master Aug 14, 2015
@itamarst itamarst deleted the constants branch August 14, 2015 20:37
@jml
Copy link
Contributor Author

jml commented Aug 17, 2015

Yeah, but it's not just a simple matter of adding .value somewhere. It's
also finding the right places to hook into the two JSON serialization
mechanisms and converting the constants into the right form wherever we may
find them.

On Fri, 14 Aug 2015 at 21:35 Itamar Turner-Trauring <
notifications@github.com> wrote:

You'd need to use STATUS.SUCCEEDED.value to prevent the serialization
problems.


Reply to this email directly or view it on GitHub
#200 (comment).

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

2 participants