-
Notifications
You must be signed in to change notification settings - Fork 10
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
Proposed fix for issue #1: autofix option #4
Conversation
end | ||
end | ||
|
||
File.open("#{src_dir}/#{src}", 'w') do |contents| |
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.
Should probably DRY this up a bit
I also noted in the README that this feature is experimental, as I haven't extensively tested it. |
Hi @ericqweinstein, I tested it on my project and I don't think the ruby code embedded in the |
@SijiaDavis Here are the steps I'm using to repro; could you confirm whether this does/does not work for you?
<p>
<% test = true; puts 'Hello!' if !test %>
</p>
$ gem build ruumba.gemspec && gem install ruumba-0.1.2.gem
$ ruumba --auto-correct . When you open example.erb, you should now see: <p>
<% test = true; puts 'Hello!' unless test %>
</p> |
lib/ruumba/analyzer.rb
Outdated
# @TODO: This is pretty hacky. (EQW 30 Aug 2017) | ||
out << if line.match?(/<%/) | ||
" <% #{code.shift.rstrip} %>\n" | ||
elsif /<%=/ |
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.
This should be:
elsif line.match?(/<%=/)
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.
@SijiaDavis This could be the source of your trouble; I'll push up a correction tonight or tomorrow.
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.
Nice catch 👍🏽, unfortunately I don't think this is the source of why it wasn't working for me... I have some <% ... %>
in my erb files that were supposed to be corrected but weren't...
@@ -33,8 +33,8 @@ def run(dir = ARGV) | |||
# @return [String] The extracted Ruby code. | |||
def extract(filename) | |||
File.read(filename).scan(ERB_REGEX).map(&:last) | |||
.reject { |line| line[0] == '#' } | |||
.map(&:strip).join("\n") | |||
.reject { |line| line[0] == '#' } |
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.
Not sure why RuboCop bumped the indentation here
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.
probably to align with .read
Hi @ericqweinstein, I followed your step in your last comment and it worked (fixed the file in the same directory), however, it's not working for me when I try to use the ruumba gem in my other projects. What I did:
Let me know if I am missing something, thanks! |
@SijiaDavis There might have been an interpolation bug; I pushed up a fix for it. Could you give it another try? |
def run_rubocop(target, tmp) | ||
args = (@options[:arguments] || []).join(' ') | ||
todo = tmp + '.rubocop_todo.yml' | ||
|
||
system("cd #{tmp} && rubocop #{args} #{target}").tap do | ||
FileUtils.cp(todo, ENV['PWD']) if File.exist?(todo) | ||
autofix!(target, tmp) if @options[:arguments]&.include? '--auto-correct' |
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.
This is not compatible with old versions of ruby :(
Could be this implemented without the &.
operator?
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.
Sure, I can downgrade this branch back to 2.2.2+ rather than 2.3.0+
@ericqweinstein I am having the same issue of @SijiaDavis |
@ceritium @SijiaDavis Got it. Will take a look early next week (apologies, out of town for a wedding this weekend) |
@ceritium @SijiaDavis Apologies, between work and school, I'm a bit swamped this week. Please feel free to pick this up and run with it if you like; otherwise, I'll get to it as soon as I can 👍 |
Initial stab at a fix for #1. Comments/discussion welcome!