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

Rename 'print' command #11

Closed
pylipp opened this issue Sep 17, 2019 · 12 comments · Fixed by #40
Closed

Rename 'print' command #11

pylipp opened this issue Sep 17, 2019 · 12 comments · Fixed by #40

Comments

@pylipp
Copy link
Owner

pylipp commented Sep 17, 2019

The effect of the command is printing of a formatted period to the terminal, however most commands result in printing some text. Something like get-many might suit better.

@babhishek21
Copy link
Contributor

@pylipp I think show would be a better alternative to print.

@pylipp
Copy link
Owner Author

pylipp commented Oct 10, 2019

Thanks for the suggestion! I still find it a bit ambiguous since e.g. there's also the get command which shows a single entry. With the renamed command, many entries are shown in a tabular format.

@babhishek21
Copy link
Contributor

babhishek21 commented Oct 10, 2019

@pylipp Honestly, list would have been perfect. Throw in a --period argument and you achieve

  • support print at date other than today

Maybe we could also rename the current list command to show (which is the command most SQL systems follow to list tables, indexes and databases). That way we free up list to replace print.

Also, rm is a not really a good choice of name for the remove command. If you're making the user type update, you might as well ask them to explicitly type remove too (both the commands have very low usage semantics).

@pylipp
Copy link
Owner Author

pylipp commented Oct 10, 2019

I like list instead of print very much. It complies well with the implementation which comes from the listing module.

I'm not familiar with terms around the SQL ecosystem, so show instead of the current list does not feel that intuitive to me. What do you think about the explicit databases, or periods (which displays a list of what you can specify using the --period option on other commands).

The inspiration for rm came from the Linux rm, and the git command. I'm fine with changing it to the explicit remove.

Note: with

[ ] support print at date other than today

I talk about a feature that shows the content of the list at a point in the past (like a snapshot). Honestly I don't know how this idea appeared :D maybe it's better to make the --filters option more powerful.

@babhishek21
Copy link
Contributor

babhishek21 commented Oct 11, 2019

What do you think about the explicit databases, or periods (which displays a list of what you can specify using the --period option on other commands).

This makes sense to me. 👍

I talk about a feature that shows the content of the list at a point in the past (like a snapshot). Honestly I don't know how this idea appeared :D maybe it's better to make the --filters option more powerful.

Ohh! I misunderstood this feature request. I thought you just wanted to extend the current print command to other periods.

So, maybe we should make milestones (or would you like to track in separate issues):

  • Make databases command
  • Rename print to list and rm to rename
  • Make periods command

@pylipp
Copy link
Owner Author

pylipp commented Oct 11, 2019

Milestones is a great idea! I see them like this:

  • rename list to periods
  • rename print to list
  • rename rm to remove
    There's no need to make new commands, everything's already there in one way or the other :D

@babhishek21
Copy link
Contributor

Awesome! @pylipp Can you add the milestones to this issue, and assign it to me? I'll start on this.

@pylipp
Copy link
Owner Author

pylipp commented Oct 12, 2019

not sure whether I did it right :'D

@babhishek21
Copy link
Contributor

Awesome! I'll add new issues / PRs to the milestone as and when needed.

@babhishek21
Copy link
Contributor

@pylipp So I tried setting up the package and running the tests. Some of your tests are fragile / not cross-platform.

On windows I had 10 tests failing, on MacOS I had 2. The ones failing on Mac are:

======================================================================
FAIL: test_invalid_host (test.test_httprequests.HttpRequestProxyTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/abhibha/misc/financeager/test/test_httprequests.py", line 51, in test_invalid_host
    self.assertIn("Name or service not known", error_message)
AssertionError: 'Name or service not known' not found in "Error sending request: HTTPConnectionPool(host='weird.foodomain.nope', port=80): Max retries exceeded with url: /periods/2000/standard/1 (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x107d51e90>: Failed to establish a new connection: [Errno 8] nodename nor servname provided, or not known'))"

======================================================================
FAIL: test_verbose (test.test_cli.CliLocalServerTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/Cellar/python/3.7.4_1/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/mock.py", line 1209, in patched
    return func(*args, **keywargs)
  File "/Users/abhibha/misc/financeager/test/test_cli.py", line 150, in test_verbose
    "Loading custom config from {}".format(TEST_CONFIG_FILEPATH)))
AssertionError: False is not true

----------------------------------------------------------------------

I'll open a separate issue to address those.

@pylipp
Copy link
Owner Author

pylipp commented Oct 15, 2019

Oh that's a pity! I purely tested on Linux distros...
For a workaround, you can probably open a PR; pushes to the branch then trigger builds on travis CI.

However, the first test fails for me whenever I don't have an internet connection... For the second one I'd have to have a closer look.

Would using a Docker container be an option for you?

@babhishek21
Copy link
Contributor

No worries. I managed to find viable fixes (see #36). I'll put up a separate PR for that.

babhishek21 pushed a commit to babhishek21/financeager that referenced this issue Oct 16, 2019
+ `print` command renamed to `list`
+ `list` command renamed to `periods`
Part of fix for pylipp#11.
pylipp pushed a commit that referenced this issue Oct 16, 2019
- `print` command renamed to `list`
- `list` command renamed to `periods`
Part of fix for #11.
babhishek21 pushed a commit to babhishek21/financeager that referenced this issue Oct 18, 2019
pylipp pushed a commit that referenced this issue Oct 20, 2019
Fixes #11.
All other commands have to be 'fully' typed, hence it's consistent to change 'rm'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants