dduugg / yard-sorbet

Types are documentation
Apache License 2.0
32 stars 9 forks source link

Caching not reloading docstring when using Sorbet signature #201

Closed Korri closed 1 year ago

Korri commented 1 year ago

Given the following code:

class TestClass
  # This is some neat method!
  sig { returns(T::Boolean) }
  def method
    true
  end
end

Changing the comment (let's say we remove "neat" from there), requires the use of yardoc --no-cache flag if the file is already generated, same issue if using yard server --reload, which doesn't detect the changes of comments above the Sorbet signature.

The issue is gone if I remove the Sorbet signature, even when using the plugin:

class TestClass
  # This is some neat method!
  sig { returns(T::Boolean) 
  def method
    true
  end
end

Now if I remove "neat" and run yardoc the file will get updated properly.

dduugg commented 1 year ago

Hi @Korri, thanks for filing this.

IIUC, the idea of the YARD cache is to, for example, allow re-generation of docs with with another template (as mentioned in the YARD README). I wouldn't expect it to work if the source code has changed, it makes sense that one need to discard the cache in that case.

Does it work the way you expect before this change? That was to allow parsing of sigs in rbi files, and attach them to existing documentation (so that one didn't need to choose between source code documentation and rbi sig documentation for a method).

If so, I would consider a PR that enables the prior behavior behind a flag, something like --overwrite. Does that seem reasonable?

Korri commented 1 year ago

@dduugg sorry I was still missing some context when I wrote the issue initially. We use --use-cache .yardoc inside our .yardopts file (codebase is huge, re-generating everything takes ~10 minutes).

This works exactly as expected when using yardoc, and changing the source code will update just the piece of the doc that changed (with some limitation, renaming methods is janky for example, but nothing problematic).

With a sorbet signature though, the only way to get the code to update when I change the docstring, is to rm -rf .yardoc to completely clear the cache.

For the yardoc command one workaround is to use --no-cache (which basically clears the cache before running), but that's not an option for yard server (with or without --reload) which always uses the cache, the only workaround here is to delete the .yardoc folder (restarting the server still shows the old version).

I did a test, and the issue already exists on 0.7.0 so I don't think that it's related to https://github.com/dduugg/yard-sorbet/pull/170

Korri commented 1 year ago

Update: Seems like there is more stuff going on. Give me a little time I'll create a repository to reproduce this reliably.

Korri commented 1 year ago

Ok I think I got the issue reliably reproduced in this repository: https://github.com/Korri/yard-sorbet-issue-201/

And you were right, with this simple example I was better able to test, and reverting to version 0.7.0 of yard-sorbet does solve the issue!

dduugg commented 1 year ago

Thanks for the add'l context, @Korri. I should be able to change things so that the v0.7.0 behavior is used when --use-cache is detected. Let me know you'd prefer a different option flag to enable the behavior. (As this behavior could potentially change again, should someone wanted to use the yard cache with the current behavior.)

Korri commented 1 year ago

@dduugg in an ideal we would get rbi merging even when cache is enabled! But for us, we don't currently include rbis in our yard generation so it seems like a good compromise as a first fix. Especially because we only use caching in the development environment, and it's not an issue for the actual final documentation generation.

dduugg commented 1 year ago

@Korri PTAL at https://github.com/dduugg/yard-sorbet/pull/202 and lmk if that resolves the problem

Also, thanks for putting together the repro repository, that was super helpful and easy to use in trying to understand the issue. 🫶