-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support proper parse the file name in mingw platform. #1014
Changes from all commits
f93a7e8
215fb85
90bff2f
4459652
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 |
---|---|---|
|
@@ -12,6 +12,22 @@ class Serializer | |
include Configuration | ||
include Associations | ||
|
||
|
||
# Matches | ||
# "c:/git/emberjs/ember-crm-backend/app/serializers/lead_serializer.rb:1:in `<top (required)>'" | ||
# AND | ||
# "/c/git/emberjs/ember-crm-backend/app/serializers/lead_serializer.rb:1:in `<top (required)>'" | ||
# AS | ||
# c/git/emberjs/ember-crm-backend/app/serializers/lead_serializer.rb | ||
CALLER_FILE = / | ||
\A # start of string | ||
\S+ # one or more non-spaces | ||
(?= # stop previous match when | ||
:\d+ # a colon is followed by one or more digits | ||
:in # followed by a colon followed by in | ||
) | ||
/x | ||
|
||
class << self | ||
attr_accessor :_attributes | ||
attr_accessor :_attributes_keys | ||
|
@@ -29,8 +45,7 @@ def self.inherited(base) | |
base._attributes = self._attributes.try(:dup) || [] | ||
base._attributes_keys = self._attributes_keys.try(:dup) || {} | ||
base._urls = [] | ||
serializer_file = File.open(caller.first[/^[^:]+/]) | ||
base._cache_digest = Digest::MD5.hexdigest(serializer_file.read) | ||
base._cache_digest = digest_caller_file(caller.first) | ||
super | ||
end | ||
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. needs a test! 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. 😈 (yay!) 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. OK, be seriously, there is already testing existing, but the problem is if you not running in Windows, c:/git won't return to you from ruby side, so it's really hard to write a test to testing that case in Linux. 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. @bf4 So if you really care about Windows user, just merge this PR and I will propose another to fix testing suite in windows, because even current testing case is not all pass in windows... 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.
Of course I care. No reason not add a regression test Also, I'm not a collab so I can't merge it sorry :) @kurko @joaomdmoura Let's see if we can set up an appveyor account to test on windows. I know RSpec does this and it works pretty well. 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. using the appveyor is right, since current testing case running in windows can already cover this problem. But seems its complex and really necessory? Just relay on me should enough, I will definitely working on windows and always report/fix issue if found. As Windows user, really don't expect we will be regarded as first class support tie like Mac/Linux in Rails ecosystem. 😭 |
||
|
||
|
@@ -161,6 +176,12 @@ def self.serializers_cache | |
@serializers_cache ||= ThreadSafe::Cache.new | ||
end | ||
|
||
def self.digest_caller_file(caller_line) | ||
serializer_file_path = caller_line[CALLER_FILE] | ||
serializer_file_contents = IO.read(serializer_file_path) | ||
Digest::MD5.hexdigest(serializer_file_contents) | ||
end | ||
|
||
attr_reader :options | ||
|
||
def self.get_serializer_for(klass) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,11 @@ def test_generates_attributes_and_associations | |
def test_with_no_attributes_does_not_add_extra_space | ||
run_generator ["account"] | ||
assert_file "app/serializers/account_serializer.rb" do |content| | ||
assert_no_match /\n\nend/, content | ||
if RUBY_PLATFORM =~ /mingw/ | ||
assert_no_match /\r\n\r\nend/, content | ||
else | ||
assert_no_match /\n\nend/, content | ||
end | ||
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. @Eric-Guo you can just 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. Can not write like
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. Or if you want to get wild # record_separator = $-0 == $/
assert_no_match /#{record_separator}#{record_separator}end/, content 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. |
||
end | ||
end | ||
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.
jruby doesn't either? or jruby on windows?
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.
@bf4 it's just copy from Railties app template, not known jruby status.
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.
Oh, then that works for me. Since we don't have a Gemfile.lock in version control, we don't even need to edit that.