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

Implement a primitive version of the Argparse module #131

Merged
merged 12 commits into from
May 1, 2024
Merged

Implement a primitive version of the Argparse module #131

merged 12 commits into from
May 1, 2024

Conversation

angelcaru
Copy link
Contributor

No description provided.

@Almas-Ali
Copy link
Member

Is it usable now?

@angelcaru
Copy link
Contributor Author

Yes, check the example (examples/args_test.rn)

@Almas-Ali
Copy link
Member

I can't use it like this examples/args_test.rn --version.

@angelcaru
Copy link
Contributor Author

That's because the argument list is hardcoded in the example. Change it to sys_args() (or some slice of that that skips the initial Python program args)

@angelcaru
Copy link
Contributor Author

Oh, wait. The interpreter tries to recognize the arguments passed to the program as its own (and fails). So we can't pass command-line arguments to Radon programs yet.

@Almas-Ali
Copy link
Member

Yeah, Python is detecting those arguments and parsing with it's internal Argparse module which is used in radon.py.

@angelcaru
Copy link
Contributor Author

That means we probably need to stop using it ourselves. The CLI isn't that complex so we should be able to rewrite it without argparse

@Almas-Ali
Copy link
Member

Yeah, you can try that.

examples/args_test.rn Outdated Show resolved Hide resolved
@Almas-Ali
Copy link
Member

Can you implement a basic CLI program for demo with all features included which is implemented till now.

@angelcaru
Copy link
Contributor Author

ok

@angelcaru
Copy link
Contributor Author

I made it

@Almas-Ali
Copy link
Member

--help it not working

@Almas-Ali
Copy link
Member

--max-lines issue

@angelcaru
Copy link
Contributor Author

Elaborate

@Almas-Ali
Copy link
Member

I think we need to handle them from Argparse module side.

@angelcaru
Copy link
Contributor Author

Yeah but what is the issue?

@Almas-Ali
Copy link
Member

Elaborate

Oops, it was my mistake with passing line limit.

@Almas-Ali
Copy link
Member

But without a value it is crashing...

@Almas-Ali
Copy link
Member

Wrong file name is crashing.

@angelcaru
Copy link
Contributor Author

I know. This is just a PoC of the Argparse, no need to fix that

@Almas-Ali
Copy link
Member

Then we need to move all those crash issue to main module, so any user can use it without any issues.

@angelcaru
Copy link
Contributor Author

What are you talking about?

@angelcaru
Copy link
Contributor Author

It's just an example, there's no intent for it to be used as an actual alternative to grep

@Almas-Ali
Copy link
Member

It's just an example, there's no intent for it to be used as an actual alternative to grep

Cool down. I am asking to merge crashing issue which we are handling manually in to Argparse. So, it can handle those things it self.

@angelcaru
Copy link
Contributor Author

What crashing issue? You only mentioned it crashing on non-existent files, which is not a concern of Argparse

@Almas-Ali
Copy link
Member

I am not asking that. If no value is passed, it will send warning by it self. Also the argv will be called from inside of Argparse, we just need to call parser.parse(). This will be more abstract.

@angelcaru
Copy link
Contributor Author

OK, that makes sense now

@Almas-Ali Almas-Ali merged commit fc16027 into radon-project:master May 1, 2024
1 check passed
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.

2 participants