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

Dont annotate errors to files #17

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@westonganger
Copy link
Contributor

commented Feb 7, 2019

Solves #11

  • Dont annotate errors to files
  • Squelch errors by default
  • Use config.debug = true to puts errors

Note: this is untested so far I merely wrote it.

@7even
Copy link
Owner

left a comment

I think this needs some tests.

Show resolved Hide resolved lib/active_record/annotate.rb Outdated
Show resolved Hide resolved lib/active_record/annotate/file.rb

@westonganger westonganger force-pushed the westonganger:issue_11 branch from c19c6f9 to a27461d Feb 7, 2019

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

Tests added.

backticks = '# ```'
annotation.unshift(backticks).push(backticks)

if annotation.first.start_with?("# create_table")

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

@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?

This comment has been minimized.

Copy link
@Morozzzko

Morozzzko Feb 11, 2019

Contributor

I'll check and tell you by Tuesday evening

This comment has been minimized.

Copy link
@Morozzzko

Morozzzko Feb 13, 2019

Contributor

Sorry I'm late. It says create_table even if it's a view

Probably because it's a materialized view, not sure

create_table :table_name id: false, force: :cascade do |t|

end

@lines.unshift(*annotation, nil)
elsif configurator.debug

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

I think this should be

Suggested change
elsif configurator.debug
elsif configurator.debug?
let(:configurator) {
c = ActiveRecord::Annotate::Configurator.new
c.debug = true
c

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

This can be simplified to ActiveRecord::Annotate::Configurator.new.tap { |c| c.debug = true }.

expect(new_file).not_to be_changed
end
end

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

Redundant empty line.

### Doesnt add new annotation to non-annotated file
new_file.annotate_with(["error"], configurator)
expect(new_file).not_to be_changed
end

This comment has been minimized.

Copy link
@7even

7even Feb 10, 2019

Owner

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.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

I have some other more important tasks on the go and you seem to have lots of comments and insight. Do you have any interest in picking up this PR and finishing it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.