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

Mock note #133

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Mock note #133

merged 2 commits into from
Apr 14, 2020

Conversation

JEuler
Copy link
Collaborator

@JEuler JEuler commented Apr 10, 2020

@JEuler JEuler requested a review from lejard-h April 10, 2020 13:48
@stewemetal stewemetal self-requested a review April 10, 2020 20:33
@stewemetal
Copy link
Collaborator

stewemetal commented Apr 10, 2020

We should consider including the code sample itself in the documentation, not just the link to a comment on an issue. The comment can be referenced nontheless, but I suggest putting the code sample up front to the reader and avoiding that one indirection. :)

Also, the sample code contains this piece:

Map result = mockData[request.url.path]?.firstWhere((d) {
          if (d['id'] == request.url.queryParameters['id']) return true;
          return false;
        }, orElse: () => null);

That one function parameter called d bugs me. One-letter "names" are not names. It seems to me that something like mockQueryParams would be a more suitable name.

@JEuler
Copy link
Collaborator Author

JEuler commented Apr 11, 2020

I am okay with oneLetter names for some cases (indexes, short code blocks) but for this code mockQueryParams is okay! :)

I fixed!

@stewemetal
Copy link
Collaborator

I agree, it's OK for some well-defined cases, like index variables.

Thanks for the fix! :]

Copy link
Collaborator

@stewemetal stewemetal left a comment

Choose a reason for hiding this comment

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

LGTM, @lejard-h please take a look! :)

Copy link
Owner

@lejard-h lejard-h left a comment

Choose a reason for hiding this comment

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

LGTM !

@JEuler JEuler merged commit d48851f into develop Apr 14, 2020
@JEuler JEuler deleted the mock-note branch April 14, 2020 10:55
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.

3 participants