Skip to content

Conversation

sato11
Copy link
Contributor

@sato11 sato11 commented Jun 10, 2021

This commit partially reverts #801, which by declaring conditional return
has turned get_loaded_model_by_path to a less safe method that can
return nil when its condition is not met.

Apparently the very same condition has been brought to annotate_model_file
by #774, which seems to cover the "bug" insisted in #801 as well.

On the other hand #801 has brought an inconvenient behaviour as well:
whenever a non-activerecord model file is found, get_loaded_model_by_path
returns nil, which leads to raising BadModelFileError and ends up
printing a bunch of "Unable to annotate ..." messages.

Now it seems tests added by #801 are running right and I do not find
a problem restoring the previous behaviour and turn it nil-safe again.


Say we have a file like app/models/foo/bar.rb which is NOT a subclass of ActiveRecord::Base:

class Foo::Bar end

We do not want this to get annotated but the current code does give it a try, ending up showing a bunch of error messages like this:

Unable to annotate app/models/foo/bar.rb: file doesn't contain a valid model class Unable to annotate app/models/foo/baz.rb: file doesn't contain a valid model class ... 

With this change the behaviour in v3.1.1 is restored and these un-informational errors are gone.

This commit partially reverts #801, which by declaring conditional return has turned `get_loaded_model_by_path` to a less safe method that can return nil when its the condition is not met. Apparently the very same condition has been brought to `annotate_model_file` by #774, which seems to cover the "bug" insisted in #801 as well. On the other hand #801 has brought an inconvenient behaviour as well: whenever a non-activerecord model file is found, `get_loaded_model_by_path` returns nil, which leads to raising `BadModelFileError` and ends up printing a bunch of "Unable to annotate ..." messages. Now it seems tests added by #801 are running right and I do not find a problem restoring the previous behaviour and turn it nil-safe again.
@sato11
Copy link
Contributor Author

sato11 commented Jun 10, 2021

@ctran
Will you take a look at this before proceeding with v3.1.2? (#881)
The CI's failed but I guess it's a config issue since all the PRs have failed too since mid-April?

@ctran
Copy link
Owner

ctran commented Jun 10, 2021

I'll take a look at the CI configuration. You want this to be merged for 3.1.2 right?

@ctran ctran self-assigned this Jun 10, 2021
@ctran ctran added this to the v3.1.2 milestone Jun 10, 2021
@sato11
Copy link
Contributor Author

sato11 commented Jun 11, 2021

Yes I do since v3.1.2 would introduce breaking change if shipped without this. Thanks for the quick action! 🙌

@ctran ctran merged commit e4f761c into ctran:develop Jun 14, 2021
@sato11 sato11 deleted the restore-nil-safety branch June 14, 2021 11:57
ocarta-l pushed a commit to ocarta-l/annotate_models that referenced this pull request Jun 18, 2021
This commit partially reverts ctran#801, which by declaring conditional return has turned `get_loaded_model_by_path` to a less safe method that can return nil when its the condition is not met. Apparently the very same condition has been brought to `annotate_model_file` by ctran#774, which seems to cover the "bug" insisted in ctran#801 as well. On the other hand ctran#801 has brought an inconvenient behaviour as well: whenever a non-activerecord model file is found, `get_loaded_model_by_path` returns nil, which leads to raising `BadModelFileError` and ends up printing a bunch of "Unable to annotate ..." messages. Now it seems tests added by ctran#801 are running right and I do not find a problem restoring the previous behaviour and turn it nil-safe again.
@ctran ctran modified the milestones: v3.1.2, v3.2.0 Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants