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 Pipeline implementation #87

Merged
merged 4 commits into from
Jun 23, 2017
Merged

Conversation

donny-dont
Copy link

Adds the Pipeline class and associated tests.

@donny-dont
Copy link
Author

This is dependent on #85

Enabling tests is dependent on dart-archive/package_resolver#4

@donny-dont
Copy link
Author

@nex3 trying to get some more parallelism.

/// var client = const Pipeline()
/// .addMiddleware(loggingMiddleware)
/// .addMiddleware(basicAuthMiddleware)
/// .addHandler(new Client());
Copy link
Member

Choose a reason for hiding this comment

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

addClient()

It may make sense to also add an addHandler() method, though, which automatically creates a Client.

/// .addHandler(new Client());
class Pipeline {
final Pipeline _parent;
final Middleware _middleware;
Copy link
Member

Choose a reason for hiding this comment

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

Please document these.


import 'package:http/http.dart';
import 'package:test/test.dart';
// \TODO REMOVE
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Author

Choose a reason for hiding this comment

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

I was doing the pull requests all in tandem so I didn't want to end up with a conflict with the exports. Will fix with a rebase.

// BSD-style license that can be found in the LICENSE file.

import 'package:http/http.dart';
import 'package:test/test.dart';
Copy link
Member

Choose a reason for hiding this comment

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

package: imports for other packages should be separate from package: imports for the current package.

Copy link
Author

Choose a reason for hiding this comment

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

Ok so from here on out you want the order to be

  • Other package dependencies
  • This package dependencies
  • Relative imports

Wasn't the case with the previous test files but I can modify accordingly

Copy link
Member

Choose a reason for hiding this comment

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

Yes; see the style guide.

Copy link
Author

Choose a reason for hiding this comment

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

👍


void main() {
test('compose middleware with Pipeline', () async {
int accessLocation = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Don't type-annotate local variables.

// BSD-style license that can be found in the LICENSE file.

import 'package:http/http.dart';
import 'package:test/test.dart';
Copy link
Member

Choose a reason for hiding this comment

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

Yes; see the style guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants