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

Handle no serializer source file to digest. #1271

Merged
merged 1 commit into from
Oct 16, 2015

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Oct 15, 2015

Handle no serializer source file to digest; output warning. Closes #1176

Outputs warning when no serializer source found.

Extracted from #1260

Please set `::_cache_digest` of the serializer
if you'd like to cache it.
EOF
''.freeze
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 very descriptive error message explains the problem, and says what to do about it! yay
🎉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why is the freeze on an empty string?

Or is this something special with how the <<-STRING syntax works?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just a premature optimization :)

B mobile phone

On Oct 15, 2015, at 6:25 AM, L. Preston Sego III [email protected] wrote:

In lib/active_model/serializer.rb:

@@ -27,6 +27,20 @@ class Serializer
)
/x

  • Hashes contents of file for +_cache_digest+

  • 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)
    
  • rescue TypeError, Errno::ENOENT
  •  warn <<-EOF.strip_heredoc
    
  •   Cannot digest non-existent file: '#{caller_line}'.
    
  •   Please set `::_cache_digest` of the serializer
    
  •   if you'd like to cache it.
    
  •   EOF
    
  •  ''.freeze
    
    but why is the freeze on an empty string?

Or is this something special with how the <<-STRING syntax works?


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we fixed the case where the error is spit because the file name contains a whitespace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think so

B mobile phone

On Oct 21, 2015, at 3:53 PM, Lucas Hosseini [email protected] wrote:

In lib/active_model/serializer.rb:

@@ -27,6 +27,20 @@ class Serializer
)
/x

  • Hashes contents of file for +_cache_digest+

  • 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)
    
  • rescue TypeError, Errno::ENOENT
  •  warn <<-EOF.strip_heredoc
    
  •   Cannot digest non-existent file: '#{caller_line}'.
    
  •   Please set `::_cache_digest` of the serializer
    
  •   if you'd like to cache it.
    
  •   EOF
    
  •  ''.freeze
    
    Have we fixed the case where the error is spit because the file name contains a whitespace?


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this PR fix actually? Was the problem not all along that we don't handle whitespace in filenames?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When there's no source file. More info in description and tests

B mobile phone

On Oct 21, 2015, at 6:06 PM, Lucas Hosseini [email protected] wrote:

What does this PR fix actually? Was the problem not all along that we don't handle whitespace in filenames?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but what I meant was that the bug actually arose in the case the source file was deemed non-existent because the regex would not allow for whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

B mobile phone

On Oct 21, 2015, at 6:20 PM, Lucas Hosseini [email protected] wrote:

In lib/active_model/serializer.rb:

@@ -27,6 +27,20 @@ class Serializer
)
/x

  • Hashes contents of file for +_cache_digest+

  • 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)
    
  • rescue TypeError, Errno::ENOENT
  •  warn <<-EOF.strip_heredoc
    
  •   Cannot digest non-existent file: '#{caller_line}'.
    
  •   Please set `::_cache_digest` of the serializer
    
  •   if you'd like to cache it.
    
  •   EOF
    
  •  ''.freeze
    
    Yeah but what I meant was that the bug actually arose in the case the source file was deemed non-existent because the regex would not allow for whitespace.


Reply to this email directly or view it on GitHub.

@bf4
Copy link
Member Author

bf4 commented Oct 15, 2015

@NullVoxPopuli wanna label LGTM?

@NullVoxPopuli
Copy link
Contributor

LGTM. Will merge when Travis passes

NullVoxPopuli added a commit that referenced this pull request Oct 16, 2015
Handle no serializer source file to digest.
@NullVoxPopuli NullVoxPopuli merged commit b2cd7bb into rails-api:master Oct 16, 2015
@bf4 bf4 deleted the fix_digest_failure branch June 14, 2016 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants