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

Support proper parse the file name in mingw platform. #1014

Closed
wants to merge 4 commits into from

Conversation

@Eric-Guo
Copy link
Contributor

commented Jul 17, 2015

Original regexp will failed the file name parse in Windows.

Example link: http://rubular.com/r/QRJfZ11BZL

Eric-Guo added a commit to Eric-Guo/ember-crm-backend that referenced this pull request Jul 17, 2015

@bf4

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

Please add a test using the example from your regex link

regex = /^[^:]+/
caller_line = "c:/git/emberjs/ember-crm-backend/app/serializers/lead_serializer.rb:1:in `<top (required)>'"
regex.match(caller_line) #=> #<MatchData "c">
caller_line = "c/git/emberjs/ember-crm-backend/app/serializers/lead_serializer.rb:1:in `<top (required)>'"
regex.match(caller_line) #=>  #<MatchData "c/git/emberjs/ember-crm-backend/app/serializers/lead_serializer.rb">

Also, while in here, it might be worth changing the File.open(name).read to File.read so that we don't create a lot of unclosed filehandles

@bf4

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

Looks like if you change the regex to use a lookahead it works fine \A\S+(?=:\d+:in)

CALLER_FILE = /
  /A # start of string
  \S+ # one or more non-spaces
  (?= # stop previous match when
    :\d+:in # a colon is followed by one or more digits 
               # followed by a colon followed by in
   ) 
/x

@Eric-Guo Eric-Guo force-pushed the Eric-Guo:master branch from 342901f to 151301c Jul 17, 2015

@Eric-Guo Eric-Guo changed the title Using parse_caller to support proper parse the file name in mingw platform. To support proper parse the file name in mingw platform. Jul 17, 2015

@Eric-Guo Eric-Guo changed the title To support proper parse the file name in mingw platform. Support proper parse the file name in mingw platform. Jul 17, 2015

@Eric-Guo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2015

@bf4 I just change code to using only CALLER_FILE pattern, also using File.read.

it's a bit too hard for me to add a testing, so I just write commit comments instead. 😄

serializer_file = File.open(caller.first[/^[^:]+/])
base._cache_digest = Digest::MD5.hexdigest(serializer_file.read)
serializer_file_path = caller.first[/\A\S+(?=:\d+:in)/]
base._cache_digest = Digest::MD5.hexdigest(File.read(serializer_file_path))
end

This comment has been minimized.

Copy link
@bf4

bf4 Jul 17, 2015

Member

needs a test!

This comment has been minimized.

Copy link
@bf4

bf4 Jul 17, 2015

Member

😈 (yay!)

This comment has been minimized.

Copy link
@Eric-Guo

Eric-Guo Jul 17, 2015

Author Contributor

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.

This comment has been minimized.

Copy link
@Eric-Guo

Eric-Guo Jul 17, 2015

Author Contributor

@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...
error

This comment has been minimized.

Copy link
@bf4

bf4 Jul 18, 2015

Member

if you really care about Windows user

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.

This comment has been minimized.

Copy link
@Eric-Guo

Eric-Guo Jul 18, 2015

Author Contributor

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. 😭

@joaomdmoura

This comment has been minimized.

Copy link
Member

commented Jul 19, 2015

Hey ppl. First of all @Eric-Guo we care about all users.
I'm already setting an appveyor account in order to improve our stack.
@bf4 is right, I'd love to have as many tests as possible for the right things.
There is no reason to rush here, you can keep using your fork while we set up appveyor and write a test for it.

@Eric-Guo Eric-Guo force-pushed the Eric-Guo:master branch 3 times, most recently from 58dc096 to 7e5e2b4 Jul 20, 2015

@Eric-Guo Eric-Guo force-pushed the Eric-Guo:master branch 3 times, most recently from c5cee81 to 5f131b8 Jul 29, 2015

@bf4

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

@bf4

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

I'm happy to pair or pull in your commit

@Eric-Guo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2015

Sorry, seems after code rebase, some of the comments lost, the existing serialization_scope_name_test can already cover this change, the only problem is caller will return 'c:\git\path' only in Windows, so you can not catch it if you running test suite in Linux/Mac. 😅

@Eric-Guo Eric-Guo force-pushed the Eric-Guo:master branch 3 times, most recently from 8c471cd to f319d40 Aug 7, 2015

@Eric-Guo Eric-Guo force-pushed the Eric-Guo:master branch from f319d40 to f477666 Aug 15, 2015

need lookahead match for windows file path contain 'c:/git/'
So we will got full file path instead of only c if caller.first is: c:/git/emberjs/ember-crm-backend/app/serializers/lead_serializer.rb:1:in `<top (required)>'

CALLER_FILE = /
  /A # start of string
  \S+ # one or more non-spaces
  (?= # stop previous match when
    :\d+:in # a colon is followed by one or more digits
               # followed by a colon followed by in
   )
/x

credit from https://gist.github.com/mikezter/540132 and @bf4

@Eric-Guo Eric-Guo force-pushed the Eric-Guo:master branch from f477666 to f93a7e8 Aug 18, 2015

@bf4

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

@Eric-Guo I made a PR into your master branch with tests. Would you be willing to merge that in so we can merge this? Eric-Guo#1

Merge pull request #1 from bf4/parse_windows_paths
Test caller line parsing and digesting
@Eric-Guo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2015

@bf4 thanks, I running in Windows and new add testing pass after add tzdata, only one testing failed.

  1) Failure:
SerializerGeneratorTest#test_with_no_attributes_does_not_add_extra_space [c:/git/emberjs/active_model_serializers/test/g
enerators/serializer_generator_test.rb:48]:
Expected /\n\nend/ to not match "\nclass AccountSerializer < ActiveModel::Serializer\n  attributes :id\n\nend\n\n".

182 runs, 273 assertions, 1 failures, 0 errors, 0 skips
@Eric-Guo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2015

@joaomdmoura appreciate could merge this now?

@@ -15,3 +15,6 @@ if version == "master"
else
gem "rails", "~> #{version}.0"
end

# Windows does not include zoneinfo files, so bundle the tzinfo-data gem
gem 'tzinfo-data', platforms: [:mingw, :mswin, :x64_mingw, :jruby]

This comment has been minimized.

Copy link
@bf4

bf4 Aug 19, 2015

Member

jruby doesn't either? or jruby on windows?

This comment has been minimized.

Copy link
@Eric-Guo

Eric-Guo Aug 20, 2015

Author Contributor

@bf4 it's just copy from Railties app template, not known jruby status.

This comment has been minimized.

Copy link
@bf4

bf4 Aug 20, 2015

Member

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.

@bf4

This comment has been minimized.

Copy link
Member

commented Aug 19, 2015

@Eric-Guo you're saying you have one test failing on windows test/generators/serializer_generator_test.rb:48 (something about matching line endings in the generator) but you're ok merging it?

@Eric-Guo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2015

@joaomdmoura @bf4 serializer_generator_test also fixed, because in windows, using \r\n instead of \n.

assert_no_match /\r\n\r\nend/, content
else
assert_no_match /\n\nend/, content
end

This comment has been minimized.

Copy link
@bf4

bf4 Aug 20, 2015

Member

@Eric-Guo you can just assert_no_match /\r?\n\r?\nend/, content

This comment has been minimized.

Copy link
@Eric-Guo

Eric-Guo Aug 21, 2015

Author Contributor

Can not write like assert_no_match /\r?\n\r?\nend/, content, because will cause below error.

  1) Failure:
SerializerGeneratorTest#test_with_no_attributes_does_not_add_extra_space [c:/git/emberjs/active_model_serializers/test/g
enerators/serializer_generator_test.rb:48]:
Expected /\r?\n\r?\nend/ to not match "\nclass AccountSerializer < ActiveModel::Serializer\n  attributes :id\n\nend\n\n"

This comment has been minimized.

Copy link
@bf4

bf4 Aug 21, 2015

Member

Or if you want to get wild

# record_separator = $-0 == $/
assert_no_match /#{record_separator}#{record_separator}end/, content

This comment has been minimized.

Copy link
@Eric-Guo

Eric-Guo Aug 21, 2015

Author Contributor

$-0 are also '\n', same as unix, maybe ruby bug?
crlf

@Eric-Guo Eric-Guo force-pushed the Eric-Guo:master branch from 4e108f0 to 2daca84 Aug 21, 2015

Make testing suite running and pass in Windows
thanks @bf4 give many help and suggestion

@Eric-Guo Eric-Guo force-pushed the Eric-Guo:master branch from 2daca84 to 4459652 Aug 21, 2015

@joaomdmoura

This comment has been minimized.

Copy link
Member

commented Aug 21, 2015

FYI: #1066

@bf4

This comment has been minimized.

Copy link
Member

commented Aug 21, 2015

Awesome!

Eric-Guo added a commit to Eric-Guo/active_model_serializers that referenced this pull request Aug 21, 2015

Make testing suite running and pass in Windows
thanks @bf4 give many help and suggestion, original PR rails-api#1014
@Eric-Guo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2015

Now PR to appveyor branch at #1071

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