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

Optionally read code from stdin #2

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

plexus
Copy link
Contributor

@plexus plexus commented Nov 1, 2019

When no arguments are given then read from stdin instead. This way you can pipe code into rep.

Also prints a nicer warning when no port number is given and it can't find .nrepl-port.

@eraserhd
Copy link
Owner

eraserhd commented Nov 5, 2019

Thanks a lot for this! I’ve only taken a quick look, but it basically looks good. Please add test cases, though. I’ll look more closely tomorrow.

Copy link
Owner

@eraserhd eraserhd left a comment

Choose a reason for hiding this comment

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

Apparently, my system is a tire fire with respect to Graal's dependencies. It'll take me a while to sort this out, and I'm going to make it use Nix to build, which should make the build reproducible. (The old way should probably work still.)

So... later this week, I think.

Meanwhile:

  • Add a CHANGELOG.adoc entry.
  • Document this feature in rep.1.adoc.
  • Add yourself to a contributors section in the README, if you like.

.circleci/images/primary/build.sh Outdated Show resolved Hide resolved
project.clj Outdated Show resolved Hide resolved
@eraserhd
Copy link
Owner

eraserhd commented Nov 7, 2019

I pushed a bunch of commits that get it building for me again. It uses Nix for local builds and CI, which keeps things reproducible, and I updated the documentation on how to build and test it. Your "fix native-image build" commit was merged, but the feature isn't.

Turns out, this feature is hanging the tests. I assume because there's a test that was doing something else and it is now being interpreted as wanting to read input, and that leads me to:

It's possible to want to evaluate the code - as getting the value of -, so we should use a separate option to specify input file, like -f <file>.

I think there's a problem here with the line numbers and -l when multiple forms are read from a file. We could probably send all the code in one message, but this disallows using this as a pipe. Is that the intent? Is it a good idea? Hrmm.

@plexus
Copy link
Contributor Author

plexus commented Nov 8, 2019

Thanks, I'll have a look at the tests next week, and will change - to -f -.

this disallows using this as a pipe. Is that the intent?

Yes, the reason I implemented this feature is that I was generating some code with babashka, and trying to pipe it to rep.

@eraserhd
Copy link
Owner

eraserhd commented Nov 8, 2019

babashka looks nice!

I just realized that it's possible to use - to accept input from stdin, although, you will have to distinguish between - before and after --, which tools.cli may not do.

When no args are given, read code from stdin instead.
Also adds a "contributors" section to the README.
Not sure if `-` worked in the first place.
@plexus
Copy link
Contributor Author

plexus commented Nov 25, 2019

Hi @eraserhd, I've rebased, added a CHANGELOG entry, updated the manpage, added a contributors section to the README, and checked in the patch.

It only reads from STDIN now if arguments is empty, i.e. if no expression is given. I figured that's good enough for now?

Thanks!

@plexus plexus requested a review from eraserhd November 30, 2019 16:44
@eraserhd
Copy link
Owner

eraserhd commented Dec 2, 2019

If you add a test, I’ll merge it.

If you can figure out how to make it preserve line numbers for code read from stdin, that would be really nice.

@eraserhd eraserhd changed the base branch from develop to main May 12, 2022 19:37
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