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

Fix annotations swallowing comments #72

Merged
merged 20 commits into from
Feb 17, 2024
Merged

Fix annotations swallowing comments #72

merged 20 commits into from
Feb 17, 2024

Conversation

drwl
Copy link
Owner

@drwl drwl commented Oct 25, 2023

This PR attempts to fix a long standing bug that was carried over from the original gem. Comments near an annotation would get removed when adding or updating annotations.

ctran/annotate_models#951, https://github.com/ctran/annotate_models/#warning

Context
Prior to this change, the AnnotateRb and the old gem Annotate would annotate model files in 1 of 2 ways:

  1. The "update" method, existing annotations would be found using a regex and then new annotations would be changed in place. If a human made comment was added after the annotation block without a new line, it would get removed during the update process.
  2. The "overwrite" method, which would happen when annotations did not exist OR if the :force option was set to true. A regex would be used to find annotation, remove it from the file, and then add annotations again to the file depending on options such as :position.

Relying on regexes to identify annotations in a file made it hard to reason about and hard to change, so most of them were removed and replaced with a custom Ruby file parser using Ripper [1], which makes working with Ruby files deterministic.

This PR makes the following behavioral changes:

  • When updating annotations (i.e. annotations exist && force: false), it only updates the annotations and should no longer delete any human comments that are before or after an annotation block
  • When generating (no annotations exist in the file) or re-generating annotations (force: true), it removes the annotation block and surrounding whitespace, if it exists, and then depending on the position option, will add annotations before the class declaration or after the class declaration. In the case of re-generating annotations, any surrounding whitespace will be preserved assuming annotations are being written to the same position.

[1] A great write up on Ripper: https://kddnewton.com/2022/02/14/formatting-ruby-part-1.html

@drwl drwl force-pushed the drwl/fix-comment-bug branch 3 times, most recently from a697ebb to c08f6a2 Compare February 14, 2024 08:19
@drwl
Copy link
Owner Author

drwl commented Feb 17, 2024

This PR is quite big and a good amount of the lines added are due to test cases I added to document the behavior when annotating a single file. There's a bit of polish left to do in terms of refactors, but I will add that in a future PR.

@drwl drwl merged commit 22675b2 into main Feb 17, 2024
18 checks passed
@drwl drwl deleted the drwl/fix-comment-bug branch February 17, 2024 16:56
drwl added a commit that referenced this pull request Feb 18, 2024
This PR adds support for other Ruby files types that should be able to
get model annotations added to them (i.e. factories, fabricators, etc).

In #72, AnnotateRb was changed to use `FileParser::CustomParser` to
parse Ruby files instead of relying on regexes. That PR only added
support for model files that used either `module Namespace...` or `class
ModelName...` as the first line of Ruby. The parser did not have support
for other file structures like FactoryBot factories or Fabrication
fabricators.

There may be other Ruby files that have code that is not currently
handled by `CustomParser`, so those will have to be added in the future.
drwl added a commit that referenced this pull request May 14, 2024
This PR fixes adding annotation to YML files, like fixtures. The Ruby
parser introduced in #72 and #82 did not support YML files.
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.

None yet

1 participant