Validating CLI arguments #561

tl3119 · 2023-05-31T16:30:58Z

Describe your changes

Add checking for unrecognized arguments and a test

Issue number

Closes #531

Checklist before requesting a review

Performed a self-review of my code
Formatted my code with pkgmt format
Added tests (when necessary).
Added docstring documentation and update the changelog (when needed)

📚 Documentation preview 📚: https://jupysql--561.org.readthedocs.build/en/561/

Validating CLI arguments

src/tests/test_magic.py

src/sql/magic.py

edublancas

hey @mehtamohit013, can you give @tl3119 a hand? he's a bit stuck on how to approach this. you can meet on Slack and brainstorm do some pair programming session

all the relevant info is in the issue description: #531

mehtamohit013 · 2023-06-01T17:55:04Z

hey @mehtamohit013, can you give @tl3119 a hand? he's a bit stuck on how to approach this. you can meet on Slack and brainstorm do some pair programming session

all the relevant info is in the issue description: #531

Yeah sure.

Btw @tl3119 are you on Slack, I can't seem to find you in Direct Messages?

tl3119 · 2023-06-01T18:07:47Z

hey @mehtamohit013, can you give @tl3119 a hand? he's a bit stuck on how to approach this. you can meet on Slack and brainstorm do some pair programming session
all the relevant info is in the issue description: #531

Yeah sure.

Btw @tl3119 are you on Slack, I can't seem to find you in Direct Messages?

Yeah, I am on Slack, I have sent you a message!

Validating CLI arguments

edublancas

once your PR is ready for review, please mark it as so (this one is still marked as a draft)

doc/intro.md

doc/environment.yml

src/sql/magic.py

src/tests/test_magic.py

edublancas

@tl3119 please request a review only when all the comments have been addressed. there are still some comments where I don't see an answer

doc/intro.md

tl3119 · 2023-06-06T15:12:59Z

All the comments and issues have been addressed now

src/tests/test_magic.py

src/sql/magic.py

src/tests/test_magic.py

Validating CLI arguments

mehtamohit013 · 2023-06-07T20:43:36Z

@edublancas lgtm

edublancas · 2023-06-07T21:28:58Z

src/sql/magic.py

+    def check_random_arguments(self, line="", cell=""):
+        # check only for cell magic
+        if cell != "":
+            tokens = line.split()


better to use shlex insted of line.split: https://docs.python.org/3/library/shlex.html#shlex.split

I just noticed we have to call if like this: shlex.split('--hello "--another"', posix=False)

so it doesn't remove any quotes inside the string

Sure, update it to use shlex now

edublancas · 2023-06-07T21:29:21Z

src/sql/magic.py

+            # If the token starts with "--", it is an argument
+            breakLoop = False
+            for token in tokens:
+                if token.startswith("--"):


there are short options as well, so we also need to check if the options start with a single dash

update it now

src/tests/test_magic.py

edublancas · 2023-06-07T21:48:05Z

src/sql/magic.py

+    def check_random_arguments(self, line="", cell=""):
+        # check only for cell magic
+        if cell != "":
+            tokens = line.split()


I just noticed we have to call if like this: shlex.split('--hello "--another"', posix=False)

so it doesn't remove any quotes inside the string

src/tests/test_magic.py

Validating CLI arguments

edublancas · 2023-06-08T19:19:58Z

great work @tl3119 - this is gonna help users a lot!

anupam-tiwari · 2023-06-08T19:30:26Z

🙌🙌🙌

tl3119 · 2023-06-08T22:12:03Z

I am happy it can help users a lot!

tl3119 added 2 commits May 31, 2023 12:29

Validating CLI arguments

ae2f54d

Merge branch 'master' of https://github.com/tl3119/jupysql

2be9e6a

edublancas marked this pull request as draft June 1, 2023 00:16

tl3119 and others added 2 commits May 31, 2023 20:37

Validating CLI arguments

38be8a0

Merge pull request #1 from ploomber/master

cd093c2

Validating CLI arguments

edublancas requested changes Jun 1, 2023

View reviewed changes

src/tests/test_magic.py Outdated Show resolved Hide resolved

src/tests/test_magic.py Show resolved Hide resolved

src/sql/magic.py Outdated Show resolved Hide resolved

edublancas requested changes Jun 1, 2023

View reviewed changes

tl3119 added 2 commits June 1, 2023 13:47

remove rec_arguments array and add tests for line magic and cell magic

a52c5e6

Merge branch 'master' of https://github.com/tl3119/jupysql

acc2f06

tl3119 and others added 3 commits June 1, 2023 14:18

Merge pull request #2 from ploomber/master

f06d7cc

Validating CLI arguments

Update docs to use --alias

08a540f

Merge branch 'master' of https://github.com/tl3119/jupysql

3fca85d

edublancas requested review from mehtamohit013 and anupam-tiwari June 5, 2023 17:37

Remove pinned pydata version

beec9e2

edublancas requested changes Jun 5, 2023

View reviewed changes

doc/intro.md Outdated Show resolved Hide resolved

doc/environment.yml Outdated Show resolved Hide resolved

src/sql/magic.py Outdated Show resolved Hide resolved

src/tests/test_magic.py Outdated Show resolved Hide resolved

tl3119 mentioned this pull request Jun 5, 2023

update docs to use --alias #538

Closed

Validating CLI arguments update

16f794f

tl3119 marked this pull request as ready for review June 5, 2023 19:56

edublancas requested changes Jun 5, 2023

View reviewed changes

doc/intro.md Outdated Show resolved Hide resolved

tl3119 added 2 commits June 5, 2023 18:15

Update PR

3055c77

Update PR, fix irrelevant issues

79efad6

mehtamohit013 suggested changes Jun 6, 2023

View reviewed changes

src/tests/test_magic.py Show resolved Hide resolved

src/sql/magic.py Outdated Show resolved Hide resolved

Update magic logic and test

7200eef

tl3119 requested a review from mehtamohit013 June 6, 2023 19:22

mehtamohit013 reviewed Jun 6, 2023

View reviewed changes

src/tests/test_magic.py Outdated Show resolved Hide resolved

tl3119 and others added 9 commits June 7, 2023 12:02

Update test_magic.py

90ed427

update PR

219a47c

update PR

553f49f

Merge pull request #3 from ploomber/master

43b35c3

Validating CLI arguments

fix check only for cell magic

7203acf

Merge branch 'master' of https://github.com/tl3119/jupysql

468f365

Merge pull request #5 from ploomber/master

a410d91

Validating CLI arguments

pass multiple arguments to pytest parameterized

68310ea

Merge branch 'master' of https://github.com/tl3119/jupysql

e5d4e9b

mehtamohit013 requested a review from edublancas June 7, 2023 20:43

edublancas requested changes Jun 7, 2023

View reviewed changes

tl3119 and others added 2 commits June 7, 2023 19:38

Update PR

cb8b636

Merge pull request #6 from ploomber/master

42e4f24

Validating CLI arguments

tl3119 requested a review from edublancas June 8, 2023 13:01

edublancas approved these changes Jun 8, 2023

View reviewed changes

edublancas merged commit 3877a53 into ploomber:master Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validating CLI arguments #561

Validating CLI arguments #561

tl3119 commented May 31, 2023 •

edited

Loading

edublancas left a comment

mehtamohit013 commented Jun 1, 2023 •

edited

Loading

tl3119 commented Jun 1, 2023

edublancas left a comment

edublancas left a comment

tl3119 commented Jun 6, 2023

mehtamohit013 commented Jun 7, 2023

edublancas Jun 7, 2023

edublancas Jun 7, 2023

tl3119 Jun 7, 2023 •

edited

Loading

edublancas Jun 7, 2023

tl3119 Jun 7, 2023

edublancas Jun 7, 2023

edublancas commented Jun 8, 2023

anupam-tiwari commented Jun 8, 2023

tl3119 commented Jun 8, 2023

Validating CLI arguments #561

Validating CLI arguments #561

Conversation

tl3119 commented May 31, 2023 • edited Loading

Describe your changes

Issue number

Checklist before requesting a review

edublancas left a comment

Choose a reason for hiding this comment

mehtamohit013 commented Jun 1, 2023 • edited Loading

tl3119 commented Jun 1, 2023

edublancas left a comment

Choose a reason for hiding this comment

edublancas left a comment

Choose a reason for hiding this comment

tl3119 commented Jun 6, 2023

mehtamohit013 commented Jun 7, 2023

edublancas Jun 7, 2023

Choose a reason for hiding this comment

edublancas Jun 7, 2023

Choose a reason for hiding this comment

tl3119 Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

edublancas Jun 7, 2023

Choose a reason for hiding this comment

tl3119 Jun 7, 2023

Choose a reason for hiding this comment

edublancas Jun 7, 2023

Choose a reason for hiding this comment

edublancas commented Jun 8, 2023

anupam-tiwari commented Jun 8, 2023

tl3119 commented Jun 8, 2023

tl3119 commented May 31, 2023 •

edited

Loading

mehtamohit013 commented Jun 1, 2023 •

edited

Loading

tl3119 Jun 7, 2023 •

edited

Loading