Skip to content
This repository has been archived by the owner on Nov 19, 2018. It is now read-only.

Display and output formatting #11

Merged
merged 11 commits into from
Sep 3, 2015

Conversation

lacostej
Copy link
Contributor

This is a PR for #8 and #9. Please review and comment on the following:

  • code style (I believe I addressed the early comments)
  • api stability
    • whether it was acceptable to rename Codes::CodesRunner.run (I believe so)
    • whether it was acceptable to remove the :urls argument to Codes::CodesRunner.run
    • whether we want to keep the --url argument or not from the CLI (I would scrap it, and also from the README)
  • documentation
    • should the --format documentation include other data (I took it from my own product)
    • whether the documentation should describe further the format options %c, %d, etc
  • functionality
    • as for the new display command, I have duplicated a few things with the download one, and there are things that could be removed (e.g. do we really need to create an output file, if yes, what should be its name) ?

When we have addressed everything, I will rebase / squash everything before you can merge.

@lacostej lacostej force-pushed the display_and_output_formatting branch from 21ea848 to 248649e Compare May 21, 2015 10:00
@lacostej
Copy link
Contributor Author

Would have loved to see that one in the latest release :)

@lacostej
Copy link
Contributor Author

lacostej commented Sep 1, 2015

@KrauseFx should we look into merging this at some point ?

@KrauseFx
Copy link
Contributor

KrauseFx commented Sep 1, 2015

Sorry for the delay, I'll take a look at this today! 👍

@lacostej lacostej force-pushed the display_and_output_formatting branch from 248649e to 092b270 Compare September 3, 2015 16:20
@lacostej
Copy link
Contributor Author

lacostej commented Sep 3, 2015

I updated to latest HEAD + added support for the new country parameter.

Code is now ready for review.

Maybe we don't need to append information about remaining codes to a file in the display method.

KrauseFx added a commit that referenced this pull request Sep 3, 2015
@KrauseFx KrauseFx merged commit 8969561 into fastlane-old:master Sep 3, 2015
@KrauseFx
Copy link
Contributor

KrauseFx commented Sep 3, 2015

Looks great, thank you so much for the pull request. Sorry it took so long for me to review it.

@lacostej
Copy link
Contributor Author

lacostej commented Sep 3, 2015

@KrauseFx no problem

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

Successfully merging this pull request may close these issues.

2 participants