Detect slice when generating

[krzykamil]

This PR introduces automatic slice detection when user runs a generator from inside a slice. Based on simple pwd output.

The detection happens in the parent Command class in the call, that is used by super by children commands.

The specs for this have been added to just the parent class and its tested as a method spec for all commands that end up using it indirectly. This is sort of a "compromise" between having a separate spec for each command (lots of maintenance involved, little benefit) and having just the spec for this method for the parent class. So it basically checks if children commands all can use this method.

Maybe another spec checking if it is used from the call method could be useful

[cllns]

This will implement #185, is that correct?

[krzykamil]

This will implement #185, is that correct?

Hey, I was about to ask for your opinion :)

Yes that is the issue I am trying to solve. I'm wondering if it is a good direction (a simple module inserted into generators), and if adding a test for each command is right, after testing the module itself?

EDIT:
During the rebase I see there were some significant changes since I started this 2-3 weeks back and then forgot about it :) will revise/rewrite soonish

[krzykamil]

I've changed the approach to instead leverage generators inheriting from
Hanami::CLI::Commands::App::Generate and calling super in their calls
I've added one example spec to action spec to see if this seems to be working. But not sure if the test is very good, cause I had to stub a lot to simulate user calling the generator from the slice subdirectory.

[krzykamil]

Those checks are happening a lot in the codebase. Wondering if a verbose, custom rspec matcher would do some good on those?

expect(an_action_to_exist(action, slice: slice))

and such
or even, with some custom chain methods

expect(an_action_to_exist(action).in_slice(slice))

I know not everyone is a fan of custom rspec matches, but I quite like them, coming from a place where I think RSpec is a big, heavy tool, that by default is just a base spec framework to be modified in according to project needs. Leveraging those custom matchers etc. is to me one of the main reasons to use RSpec and writing specs instead of tests using some smaller test framework.

[cllns]

Hmm that's an interesting idea! I say leave it for now, but let's keep it in mind as we build this out.

[cllns]

Thanks for doing this, and rebasing! I have some notes about the PR but the overall approach seems solid.

I guess we'll want to add specs for each of the commands? Or maybe have a generic command spec, so we don't have quite so much duplication?

[cllns]

I wonder if we should do this in the constructor instead?

Also, do we want to allow generating files in a slice foo when we're inside a slice bar? I'm leaning towards preventing --slice and --app when inside a slice or app/ but what do other think?

[krzykamil]

Hard to say, I'd say that if someone is in a slice, and wants to generate something for some other place (slice or main app) and they have to go out of the slice they are in, they might be annoyed (not a big hindrance but still). The pwd option for me is a shortcut for user, but explicit option overwriting the default would be the preferred behavior for me.

[kyleplump]

+1 vote for having this be in the constructor instead 🙂

if we're introducing this 'generate for where you are' concept, i dont hate the idea of limiting the --slice and --app flags to only be usable at the top level. as long as it provides feedback to the user, its good with me! (sounds like a guides update too 😈)

[timriley]

Great discussion, folks. Here's my opinion:

#call or #initialize for pwd check?

I think this pwd-based slice detection logic makes more sense to stay in #call.

#initialize is for dependencies. #call is for “doing the work.” To me, checking for the current working directory is part of the latter.

Even though our CLI invocations tend to be one-shot, I could imagine a world where e.g. some kind of REPL-like experience is built on top of CLI commands. In that kind of scenario, command objects may be longer-lived, and I'd want “current state” checks to be done when a command is invoked (which, in this hypothetical scenario, may be done many times) rather than initialised. (which would be done once).

I know this is a really subtle distinction, and it can be changed easily enough in the future either way, but in this case I'm arguing for the case of “change nothing”, which feels easy enough to follow :)

role of explicit --slice or --app flags when executed within a slice

I think the --slice and --app flags should work (and be respected) regardless of where you invoke the CLI. The falls under “principle of least surprise” IMHO. The flags will always exist for the case of executing from the root. They'll be documented. We should make sure people can depend on them, which means respecting them at all times.

If we made the flags work in some cases and not others, the CLI experience would feel more “slippery” than dependable, and we don't want that.

If a user is supplying these flags, it's because they've taken the time to type them out. They know what they want. We should respect it.

[cllns]

Hmm that's an interesting idea! I say leave it for now, but let's keep it in mind as we build this out.

[krzykamil]

I guess we'll want to add specs for each of the commands? Or maybe have a generic command spec, so we don't have quite so much duplication?

Yea thats my main problem. Adding a spec for each command for this is most thorough but then if something changes with this logic and we have to modify each of them... quite a chore. Maybe a spec in spec/unit/hanami/cli/commands/app/command_spec.rb that runs the same test for all of the commands that checks if this thing works correctly. Think I like that the most, will post it in upcoming days

[krzykamil]

@cllns I've changed the approach making the change only 2 files now (although the spec I am not 100% happy with). Care to take a look when you get the time?

[kyleplump]

makes sense to me @krzykamil! i will be following along, i'm curious where we land with regards to the --slice and --app flags as @cllns pointed out

[kyleplump]

would the guard clause above cover this conditional? might not need that here, even if there isnt any harm keeping it!

[kyleplump]

+1 vote for having this be in the constructor instead 🙂

if we're introducing this 'generate for where you are' concept, i dont hate the idea of limiting the --slice and --app flags to only be usable at the top level. as long as it provides feedback to the user, its good with me! (sounds like a guides update too 😈)

[kyleplump]

i might consider putting the cmd in the title as well, so that we dont have a bunch of tests named like this, e.g.: "works for the View command", etc

[timriley]

This is looking good, thanks @krzykamil! I've left:

• Some answers for you on the open questions
• A suggestion for more reliably matching slice roots
• A suggestion for the tests

We're nearly there! I hope that the above shouldn't be too much work :)

Thank you again for all your work on Hanami! And I'm really sorry for my delay in replying here.

[timriley]

Worth noting here that we technically allow nested slices, which I'd like to make sure we support it in anything that interacts with slices.

The approach in this method detects slices in the top-level slices/ directory only. Nested slices OTOH will be in paths like slices/admin/slices/editorial/ (see https://github.com/hanami/hanami/blob/c1efc98f2b813274920da30c73e6d71ba75705b8/spec/integration/slices/slice_loading_spec.rb for some examples).

So I wonder if a more robust approach here is to do something like this?

slices_by_root = app.slices.with_nested.each.to_h { |slice| [slice.root, slice] }
slices_by_root[current_dir]

Slices are already loaded as part of accessing the app, so it's no extra work to enumerate their roots at this stage. Plus, this code feels more straightforward and the implementation should be more reliable even if our handling of slice roots changes in the future. What do you reckon?

[timriley]

Thanks for all the thought you've been putting into how to test this, @krzykamil.

The approach we have here feels a little too low-level. It's testing an internal private method rather than the behaviour that it is meant to enable.

How about this as an approach?

Make a new spec/unit/hanami/cli/commands/app/generate/slice_detection_spec. This spec file will be dedicated to the slice detection feature only. It can contain tests that look like the other generate command tests, but it can focus on the slice detection feature only, and to do this, it only needs to exercise one generator command (view or action or whatever you choose); we can just trust that the slice detection works for all the others rather than writing heaps of redundant extra tests.

The tests that go into this file don't need to be as detailed as the other command-specific tests in the neighbouring files. It just needs to check that the right slice has been picked based on the cwd.

So I think it'd look something like this:

it "detects the slice based on the current working directory" do
  within_application_directory do
    fs.mkdir("slices/main")

    Dir.chdir("slices/main") do
      subject.call(name: "users.index")
    end

    expect(fs.exist?("slices/main/views/users/index.rb")).to be true
  end
end

Something like that. Just enough to know the slice was detected.

Then in the rest of this new test file we can test the few other scenarios we need to cover, like when we're in a slice directory but explicitly provide a different slice as an arg, when we're inside a deeper app directory that is not a slice directory, etc. Probably not too many in total.

How does that sound? Does it make sense? Happy to clarify anything :)

[krzykamil]

Thanks, it seems i maybe need to use fs.pwd (which I didn't knew was an option) and figure out some other things. I started building the new spec and it is a better way, just need a bit of time to figure out how it should all look like

[krzykamil]

Well I guess this just got far more complex cause fs.pwd returns only the working directory name (segment) (fair, given the name, but event linux system pwd returns a path), while Pathname.pwd returned a whole path leading up to the dir.

Mixing fs and Pathname leads to being forced to mock a lot
Then again, using fs only does also involve some mocks
Need to mock slices detection in specs too anyway I think. Unless I run the real command for generating a slice as part of those specs? Is this done somewhere already?

With the nested slices its a bit of a doozy

I would like to end up with a simple spec that looks like you posted, but it looks like its gonna be pretty tough :p

[timriley]

Have pushed a little spike of an alternative approach! #314

[krzykamil]

The approach to both detection and testing has been updated, thanks @timriley for your help and direction
I still had some problems with fs.exist? which has problem with detecting a file when given non-relative path. Because some specs (most) still use memory fs adapter, this caused a conflict between my spec using real files and lead to me adding the if/else branch with detection. But I think it might be a good addition anyway?

I also left your TODO comment from your spike

Prepared a small note in the docs about it too: hanami/guides#300

[timriley]

Thanks @krzykamil! I made a few adjustments (see last commit) and now it's merged!

I also added a big long comment explaining why we have to make two checks for the slice root:

            # Returns the root for the given slice name.
            #
            # This currently works with top-level slices only, and it simply appends the slice's
            # name onto the "slices/" dir, returning e.g. "slices/main" when given "main".
            #
            # TODO: Make this work with nested slices when given slash-delimited slice names like
            # "parent/child", which should look for "slices/parent/slices/child".
            #
            # This method makes two checks for the slice root (building off both `app.root` as well
            # as `fs`). This is entirely to account for how we test commands, with most tests using
            # an in-memory `fs` adapter, any files created via which will be invisible to the `app`,
            # which doesn't know about the `fs`.
            #
            # FIXME: It would be better to find a way for this to make one check only. An ideal
            # approach would be to use the slice_name to find actual slice registered within
            # `app.slices`. To do this, we'd probably need to stop testing with an in-memory `fs`
            # here.
            def detect_slice_root(slice_name)
              slice_root_in_fs = fs.join("slices", inflector.underscore(slice_name))
              return slice_root_in_fs if fs.exist?(slice_root_in_fs)

              app.root.join("slices", inflector.underscore(slice_name))
            end

It makes me think that we probably just need to stop using the in-memory fs adapter for CLI command tests. The Hanami app itself doesn't know about or use this fs object, which (as you discovered!) makes it very difficult to write commands that integrate with facilities returned from the Hanami.app.

[krzykamil]

Thanks @krzykamil! I made a few adjustments (see last commit) and now it's merged!

I also added a big long comment explaining why we have to make two checks for the slice root:

            # Returns the root for the given slice name.
            #
            # This currently works with top-level slices only, and it simply appends the slice's
            # name onto the "slices/" dir, returning e.g. "slices/main" when given "main".
            #
            # TODO: Make this work with nested slices when given slash-delimited slice names like
            # "parent/child", which should look for "slices/parent/slices/child".
            #
            # This method makes two checks for the slice root (building off both `app.root` as well
            # as `fs`). This is entirely to account for how we test commands, with most tests using
            # an in-memory `fs` adapter, any files created via which will be invisible to the `app`,
            # which doesn't know about the `fs`.
            #
            # FIXME: It would be better to find a way for this to make one check only. An ideal
            # approach would be to use the slice_name to find actual slice registered within
            # `app.slices`. To do this, we'd probably need to stop testing with an in-memory `fs`
            # here.
            def detect_slice_root(slice_name)
              slice_root_in_fs = fs.join("slices", inflector.underscore(slice_name))
              return slice_root_in_fs if fs.exist?(slice_root_in_fs)

              app.root.join("slices", inflector.underscore(slice_name))
            end

It makes me think that we probably just need to stop using the in-memory fs adapter for CLI command tests. The Hanami app itself doesn't know about or use this fs object, which (as you discovered!) makes it very difficult to write commands that integrate with facilities returned from the Hanami.app.

Thanks Tim, just a question:

This comment
# TODO: Make this work with nested slices when given slash-delimited slice names like
# "parent/child", which should look for "slices/parent/slices/child".

I had this spec

            context "with deeply nested slices existing" do
      it "determines the nested slice when inside of it" do
        Dir.chdir("slices/admin/slices/search") do
          subject.call(name: "panel")
        end

        expect(File.exist?("slices/admin/slices/search/views/panel.rb")).to be true
      end

Which I guess is passing. Doesn't it meet the case from the comment? Or am I missing something?

As for fs I agree, not being able to use app.slices in the spec with the same result as it gives in real app was a challenge

[timriley]

@krzykamil Ah yes, good question! So when I said "make it work with nested slices", what I meant was not the detection of the nested slice based on the pwd (which already works, as you point out), but rather being able to specify a nested slice via the --slice flag.

In other words, make this work:

hanami generate view panel --slice parent/child
# generates slices/parent/slices/child/views/panel.rb

Does that make sense?

This is not something we have supported so far, but with this code change being made, it felt like a sensible place to be able to note this as a TODO for the future :)

[krzykamil]

@krzykamil Ah yes, good question! So when I said "make it work with nested slices", what I meant was not the detection of the nested slice based on the pwd (which already works, as you point out), but rather being able to specify a nested slice via the --slice flag.

In other words, make this work:

hanami generate view panel --slice parent/child
# generates slices/parent/slices/child/views/panel.rb

Does that make sense?

This is not something we have supported so far, but with this code change being made, it felt like a sensible place to be able to note this as a TODO for the future :)

Got it now! Thanks :)
Also, have you seen the connected guides change hanami/guides#300?