Context attributes and multivalued headers #86

Merged
merged 3 commits into from Apr 12, 2013

Conversation

Projects
None yet
3 participants
@tbroyer
Contributor

tbroyer commented Apr 9, 2013

  • make headers multivalued
  • add getParameterValues; I find a never-null List saner than a never-empty nullable array.
  • add Context attributes, with @Attribute argument extractor.
@buildhive

This comment has been minimized.

Show comment
Hide comment
@buildhive

buildhive Apr 9, 2013

reyez » ninja #297 SUCCESS
This pull request looks good
(what's this?)

reyez » ninja #297 SUCCESS
This pull request looks good
(what's this?)

@buildhive

This comment has been minimized.

Show comment
Hide comment
@buildhive

buildhive Apr 9, 2013

reyez » ninja #298 SUCCESS
This pull request looks good
(what's this?)

reyez » ninja #298 SUCCESS
This pull request looks good
(what's this?)

@buildhive

This comment has been minimized.

Show comment
Hide comment
@buildhive

buildhive Apr 9, 2013

reyez » ninja #299 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

reyez » ninja #299 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@buildhive

This comment has been minimized.

Show comment
Hide comment
@buildhive

buildhive Apr 9, 2013

reyez » ninja #300 SUCCESS
This pull request looks good
(what's this?)

reyez » ninja #300 SUCCESS
This pull request looks good
(what's this?)

@raphaelbauer

This comment has been minimized.

Show comment
Hide comment
@raphaelbauer

raphaelbauer Apr 9, 2013

Contributor

Awesome stuff Thomas!

Three quick and minor things:

  1. Do you think it makes sense to add testcases at least in ContextImplTest of ninja-servlet against the new attributes stuff? Just to make sure stuff does not break.
  2. changelog.md in ninja-core/src/site/markdown/developer should be updated reflecting the new features
  3. And then there was a tweet recently saying "Looks like Ninja framework lacks documentation more than features (like so many OSS projects, starting with my owns)" (just kidding) - but docs are missing... they also live in ninja-core/src/site/markdown/ .

Just tell me if you got time to do that. I can handle that as well :) Apart from that really nice work.

Contributor

raphaelbauer commented Apr 9, 2013

Awesome stuff Thomas!

Three quick and minor things:

  1. Do you think it makes sense to add testcases at least in ContextImplTest of ninja-servlet against the new attributes stuff? Just to make sure stuff does not break.
  2. changelog.md in ninja-core/src/site/markdown/developer should be updated reflecting the new features
  3. And then there was a tweet recently saying "Looks like Ninja framework lacks documentation more than features (like so many OSS projects, starting with my owns)" (just kidding) - but docs are missing... they also live in ninja-core/src/site/markdown/ .

Just tell me if you got time to do that. I can handle that as well :) Apart from that really nice work.

@tbroyer

This comment has been minimized.

Show comment
Hide comment
@tbroyer

tbroyer Apr 9, 2013

Contributor

On Tue, Apr 9, 2013 at 2:15 PM, Raphael A. Bauer
notifications@github.com wrote:

Awesome stuff Thomas!

Three quick and minor things:

  1. Do you think it makes sense to add testcases at least in ContextImplTest of ninja-servlet against the new attributes stuff? Just to make sure stuff does not break.

Attributes are so simple I don't think adding tests makes sense.
I'm ambivalent about adding tests for getParameterValues or getHeaders(String).

  1. changelog.md in ninja-core/src/site/markdown/developer should be updated reflecting the new features

Will do.

  1. And then there was a tweet recently saying "Looks like Ninja framework lacks documentation more than features (like so many OSS projects, starting with my owns)" (just kidding) - but docs are missing... they also live in ninja-core/src/site/markdown/ .

Just tell me if you got time to do that. I can handle that as well :) Apart from that really nice work.

TBH, I'd prefer you update the documentation (that new "Request
Scopes" page you added yesterday). I'd be tempted to replace the
"WrappedContext in Filters" section altogether.
If it's OK for you, tell me and i'll do it; otherwise I'll let you
enhance the doc.

Contributor

tbroyer commented Apr 9, 2013

On Tue, Apr 9, 2013 at 2:15 PM, Raphael A. Bauer
notifications@github.com wrote:

Awesome stuff Thomas!

Three quick and minor things:

  1. Do you think it makes sense to add testcases at least in ContextImplTest of ninja-servlet against the new attributes stuff? Just to make sure stuff does not break.

Attributes are so simple I don't think adding tests makes sense.
I'm ambivalent about adding tests for getParameterValues or getHeaders(String).

  1. changelog.md in ninja-core/src/site/markdown/developer should be updated reflecting the new features

Will do.

  1. And then there was a tweet recently saying "Looks like Ninja framework lacks documentation more than features (like so many OSS projects, starting with my owns)" (just kidding) - but docs are missing... they also live in ninja-core/src/site/markdown/ .

Just tell me if you got time to do that. I can handle that as well :) Apart from that really nice work.

TBH, I'd prefer you update the documentation (that new "Request
Scopes" page you added yesterday). I'd be tempted to replace the
"WrappedContext in Filters" section altogether.
If it's OK for you, tell me and i'll do it; otherwise I'll let you
enhance the doc.

@raphaelbauer

This comment has been minimized.

Show comment
Hide comment
@raphaelbauer

raphaelbauer Apr 10, 2013

Contributor

On Tue, Apr 9, 2013 at 4:04 PM, Thomas Broyer notifications@github.comwrote:

On Tue, Apr 9, 2013 at 2:15 PM, Raphael A. Bauer
notifications@github.com wrote:

Awesome stuff Thomas!

Three quick and minor things:

  1. Do you think it makes sense to add testcases at least in
    ContextImplTest of ninja-servlet against the new attributes stuff? Just to
    make sure stuff does not break.

Attributes are so simple I don't think adding tests makes sense.
I'm ambivalent about adding tests for getParameterValues or
getHeaders(String).

If you don't think it makes sense - don't do it.

  1. changelog.md in ninja-core/src/site/markdown/developer should be
    updated reflecting the new features

Will do.

Cool...

  1. And then there was a tweet recently saying "Looks like Ninja
    framework lacks documentation more than features (like so many OSS
    projects, starting with my owns)" (just kidding) - but docs are missing...
    they also live in ninja-core/src/site/markdown/ .

Just tell me if you got time to do that. I can handle that as well :)
Apart from that really nice work.

TBH, I'd prefer you update the documentation (that new "Request
Scopes" page you added yesterday). I'd be tempted to replace the
"WrappedContext in Filters" section altogether.
If it's OK for you, tell me and i'll do it; otherwise I'll let you
enhance the doc.

Just replace and enhance my part. If I can contribute let me know :)

ra

Contributor

raphaelbauer commented Apr 10, 2013

On Tue, Apr 9, 2013 at 4:04 PM, Thomas Broyer notifications@github.comwrote:

On Tue, Apr 9, 2013 at 2:15 PM, Raphael A. Bauer
notifications@github.com wrote:

Awesome stuff Thomas!

Three quick and minor things:

  1. Do you think it makes sense to add testcases at least in
    ContextImplTest of ninja-servlet against the new attributes stuff? Just to
    make sure stuff does not break.

Attributes are so simple I don't think adding tests makes sense.
I'm ambivalent about adding tests for getParameterValues or
getHeaders(String).

If you don't think it makes sense - don't do it.

  1. changelog.md in ninja-core/src/site/markdown/developer should be
    updated reflecting the new features

Will do.

Cool...

  1. And then there was a tweet recently saying "Looks like Ninja
    framework lacks documentation more than features (like so many OSS
    projects, starting with my owns)" (just kidding) - but docs are missing...
    they also live in ninja-core/src/site/markdown/ .

Just tell me if you got time to do that. I can handle that as well :)
Apart from that really nice work.

TBH, I'd prefer you update the documentation (that new "Request
Scopes" page you added yesterday). I'd be tempted to replace the
"WrappedContext in Filters" section altogether.
If it's OK for you, tell me and i'll do it; otherwise I'll let you
enhance the doc.

Just replace and enhance my part. If I can contribute let me know :)

ra

@tbroyer

This comment has been minimized.

Show comment
Hide comment
@tbroyer

tbroyer Apr 10, 2013

Contributor

Finally added a section to "Request Scopes" and kept the one about WrappedContext (slightly reformulated).

Also, added @Attribute argument extractor.

Contributor

tbroyer commented Apr 10, 2013

Finally added a section to "Request Scopes" and kept the one about WrappedContext (slightly reformulated).

Also, added @Attribute argument extractor.

@buildhive

This comment has been minimized.

Show comment
Hide comment
@buildhive

buildhive Apr 10, 2013

reyez » ninja #301 SUCCESS
This pull request looks good
(what's this?)

reyez » ninja #301 SUCCESS
This pull request looks good
(what's this?)

@buildhive

This comment has been minimized.

Show comment
Hide comment
@buildhive

buildhive Apr 10, 2013

reyez » ninja #302 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

reyez » ninja #302 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@raphaelbauer

This comment has been minimized.

Show comment
Hide comment
@raphaelbauer

raphaelbauer Apr 12, 2013

Contributor

This is truly awesome!

Contributor

raphaelbauer commented Apr 12, 2013

This is truly awesome!

raphaelbauer added a commit that referenced this pull request Apr 12, 2013

Merge pull request #86 from tbroyer/context-attributes
Context attributes and multivalued headers

@raphaelbauer raphaelbauer merged commit 67295e6 into ninjaframework:develop Apr 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment