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.

Sign up for GitHub

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

Fix fromjson() to support reading from stdin #667

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+24 −1
Open

Fix fromjson() to support reading from stdin #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9fe7939
ci: Test the petl executable
yaniv-aknin May 12, 2024
60818d5
Fix fromjson() to support reading from stdin
yaniv-aknin Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test-changes.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ jobs:
run: |
python -m pip install --upgrade pip
python -m pip install --prefer-binary -r requirements-tests.txt
python -m pip install -e .

- name: Setup environment variables for remote filesystem testing
if: matrix.os == 'ubuntu-latest' && matrix.python == '3.8'
Expand Down
2 changes: 1 addition & 1 deletion petl/io/json.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from petl.util.base import data, Table, dicts as _dicts, iterpeek


def fromjson(source, *args, **kwargs):
def fromjson(source=None, *args, **kwargs):

Check warning

Code scanning / Prospector (reported by Codacy)

Keyword argument before variable positional arguments list in the definition of fromjson function (keyword-arg-before-vararg) Warning

Keyword argument before variable positional arguments list in the definition of fromjson function (keyword-arg-before-vararg)
Copy link
Member

@juarezr juarezr Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@yaniv-aknin

Code scanning / Prospector (reported by Codacy)

Keyword argument before variable positional arguments list in the definition of fromjson function (keyword-arg-before-vararg)

This wouldn't break existing code?

Maybe read_source_from_arg can be taught to read even when stdin is passed as argument as in:

    table1 = etl.fromjson(sys.stdin, header=["foo", "bar"])
    print(table1)

Copy link
Author

Choose a reason for hiding this comment

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

I might be wrong, but I don't think it should break existing code.

All these assertions pass -

>>> def f(s, *a, **kw):
...     return (s, a, kw)
... 
>>> def fn(s=None, *a, **kw):
...     return (s, a, kw)
... 
>>> assert f("hello") == fn("hello")
>>> assert f(None) == fn(None)
>>> assert f("hello", 1, 2) == fn("hello", 1, 2)
>>> assert f("hello", k=1) == fn("hello", k=1)
>>> assert f(s="hello") == fn(s="hello")
>>> assert f("hello", 1, 2, k=1, j=2) == fn("hello", 1, 2, k=1, j=2)

re. read_source_from_arg() - my interest is in the petl executable, i.e., I would love for syntax like this to work in shell: echo '{"a":2, "b":4}' | petl 'fromjson(lines=True)'.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think that:

  1. Using petl as a command line tool is an interesting use case and we should give it some love by:
    a. Adding more examples in the documentation.
    b. Mention this usage pattern in the repository Readme/Frontage
  2. Currently fromjson() source argument is not consistent with other functions like:
    a. tojson() and tojsonarrays()
    b. fromcsv()
  3. We value not breaking existing code.
    a. But I haven't had the time to check if this is the case yet.
    b. If not it will be a good change for API consistency
    c. I would love to hear more opinions about this change.

Dismissed Show dismissed Hide dismissed
Dismissed Show dismissed Hide dismissed
"""
Extract data from a JSON file. The file must contain a JSON array as
the top level object, and each member of the array will be treated as a
Expand Down
22 changes: 22 additions & 0 deletions petl/test/test_executable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from __future__ import print_function, division, absolute_import

Check warning

Code scanning / Pylintpython3 (reported by Codacy)

Missing module docstring Warning test

Missing module docstring

Check warning

Code scanning / Pylint (reported by Codacy)

Missing module docstring Warning test

Missing module docstring

import subprocess

Check warning

Code scanning / Bandit (reported by Codacy)

Consider possible security implications associated with the subprocess module. Warning test

Consider possible security implications associated with the subprocess module.

def test_executable():

Check warning

Code scanning / Pylint (reported by Codacy)

Missing function docstring Warning test

Missing function docstring

Check warning

Code scanning / Pylintpython3 (reported by Codacy)

Missing function or method docstring Warning test

Missing function or method docstring
result = subprocess.check_output("""

Check warning

Code scanning / Bandit (reported by Codacy)

Starting a process with a partial executable path Warning test

Starting a process with a partial executable path
(echo foo,bar ; echo a,b; echo c,d) |
petl 'fromcsv().cut("foo").head(1).tocsv()'
""", shell=True)

Check failure

Code scanning / Bandit (reported by Codacy)

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Error test

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell
assert result == b'foo\r\na\r\n'

Check notice

Code scanning / Semgrep (reported by Codacy)

The application was found using assert in non-test code. Note test

The application was found using assert in non-test code.

Check warning

Code scanning / Bandit (reported by Codacy)

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Warning test

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.

def test_json_stdin():

Check warning

Code scanning / Pylintpython3 (reported by Codacy)

Missing function or method docstring Warning test

Missing function or method docstring

Check warning

Code scanning / Pylint (reported by Codacy)

Missing function docstring Warning test

Missing function docstring
result = subprocess.check_output("""

Check warning

Code scanning / Bandit (reported by Codacy)

Starting a process with a partial executable path Warning test

Starting a process with a partial executable path
echo '[{"foo": "a", "bar": "b"}]' |
petl 'fromjson().tocsv()'
""", shell=True)

Check failure

Code scanning / Bandit (reported by Codacy)

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Error test

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell
assert result == b'foo,bar\r\na,b\r\n'

Check notice

Code scanning / Semgrep (reported by Codacy)

The application was found using assert in non-test code. Note test

The application was found using assert in non-test code.

Check warning

Code scanning / Bandit (reported by Codacy)

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Warning test

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
result = subprocess.check_output("""

Check warning

Code scanning / Bandit (reported by Codacy)

Starting a process with a partial executable path Warning test

Starting a process with a partial executable path
( echo '{"foo": "a", "bar": "b"}' ; echo '{"foo": "c", "bar": "d"}' ) |
petl 'fromjson(lines=True).tocsv()'
""", shell=True)

Check failure

Code scanning / Bandit (reported by Codacy)

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell Error test

subprocess call with shell=True seems safe, but may be changed in the future, consider rewriting without shell
assert result == b'foo,bar\r\na,b\r\nc,d\r\n'

Check notice

Code scanning / Semgrep (reported by Codacy)

The application was found using assert in non-test code. Note test

The application was found using assert in non-test code.

Check warning

Code scanning / Bandit (reported by Codacy)

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Warning test

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
Loading