-
Notifications
You must be signed in to change notification settings - Fork 4
Dont annotate errors to files #17
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,15 +2,16 @@ module ActiveRecord | |||||
| module Annotate | ||||||
| class File | ||||||
| attr_reader :path, :lines | ||||||
|
|
||||||
| def initialize(path) | ||||||
| @path = path | ||||||
| @content = ::File.read(path) | ||||||
| @lines = @content.split(?\n) | ||||||
| end | ||||||
|
|
||||||
| def annotate_with(annotation, configurator) | ||||||
| magic_comments = [] | ||||||
|
|
||||||
| while @lines.first =~ /^\s*#.*(coding|frozen_string_literal|warn_indent)/i | ||||||
| magic_comments.push(@lines.shift) | ||||||
| end | ||||||
|
|
@@ -19,39 +20,46 @@ def annotate_with(annotation, configurator) | |||||
| # separate magic comments from the annotation with an empty line | ||||||
| magic_comments << nil | ||||||
| end | ||||||
|
|
||||||
| while @lines.first.start_with?('#') || @lines.first.blank? | ||||||
| # throw out comments and empty lines | ||||||
| # in the beginning of the file (old annotation) | ||||||
| @lines.shift | ||||||
| end | ||||||
|
|
||||||
| if configurator.yard? | ||||||
| backticks = '# ```' | ||||||
| annotation.unshift(backticks).push(backticks) | ||||||
|
|
||||||
| if annotation.first.start_with?("# create_table") | ||||||
| if configurator.yard? | ||||||
| backticks = '# ```' | ||||||
| annotation.unshift(backticks).push(backticks) | ||||||
| end | ||||||
|
|
||||||
| @lines.unshift(*annotation, nil) | ||||||
| elsif configurator.debug | ||||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be
Suggested change
|
||||||
| ### If debugging enabled print errors, Else squelch the error | ||||||
| puts annotation | ||||||
westonganger marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| end | ||||||
|
|
||||||
| @lines.unshift(*annotation, nil) | ||||||
|
|
||||||
| @lines.unshift(*magic_comments) | ||||||
|
|
||||||
| @lines.push(nil) # newline at the end of file | ||||||
| end | ||||||
|
|
||||||
| def write | ||||||
| new_file_content = @lines.join(?\n) | ||||||
| temp_path = "#{@path}.annotated" | ||||||
|
|
||||||
| ::File.open(temp_path, 'w') do |temp_file| | ||||||
| temp_file.write(new_file_content) | ||||||
| end | ||||||
|
|
||||||
| ::File.delete(@path) | ||||||
| ::File.rename(temp_path, @path) | ||||||
| end | ||||||
|
|
||||||
| def changed? | ||||||
| @lines.join(?\n) != @content | ||||||
| end | ||||||
|
|
||||||
| def relative_path | ||||||
| path.sub(/^#{Rails.root}\//, '') | ||||||
| end | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,7 +78,11 @@ class User < ActiveRecord::Base | |
| end | ||
|
|
||
| let(:file) { ActiveRecord::Annotate::File.new(file_path) } | ||
| let(:configurator) { ActiveRecord::Annotate::Configurator.new } | ||
| let(:configurator) { | ||
| c = ActiveRecord::Annotate::Configurator.new | ||
| c.debug = true | ||
| c | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be simplified to |
||
| } | ||
|
|
||
| describe "#annotate_with" do | ||
| it "changes the lines adding the new annotation" do | ||
|
|
@@ -149,4 +153,19 @@ class User < ActiveRecord::Base | |
| expect(file.relative_path).to eq('namespace/path.rb') | ||
| end | ||
| end | ||
|
|
||
| describe "Annotation Errors" do | ||
| it "Removes Old Annotation" do | ||
| file.annotate_with(["error"], configurator) | ||
| expect(file).to be_changed | ||
|
|
||
| file.write | ||
| new_file = ActiveRecord::Annotate::File.new(file.path) | ||
|
|
||
| ### Doesnt add new annotation to non-annotated file | ||
| new_file.annotate_with(["error"], configurator) | ||
| expect(new_file).not_to be_changed | ||
| end | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get the idea of this test. You are re-annotating the file with the same annotation and then checking that it didn't change :) Also the title assumes that old annotation should get removed which is not what happens here. |
||
| end | ||
|
|
||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant empty line. |
||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Morozzzko will this work for views, or we need something like
annotation.first.start_with?("# create_table") || annotation.first.start_with?("# create_view")? Can you tell us how the views look like in your app's annotations?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check and tell you by Tuesday evening
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm late. It says
create_tableeven if it's a viewProbably because it's a materialized view, not sure
create_table :table_name id: false, force: :cascade do |t|