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

Extract ExceedsMaxError as TooManyCellsError #248

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

reshleman
Copy link
Contributor

As a follow-up to df53d9a, this commit
extracts Roo::Excelx::ExceedsMaxError into a class that inherits from
Roo::Error.

The new class is renamed as Roo::TooManyCellsError in order to clarify
which maximum is being exceeded.

As a follow-up to df53d9a, this commit
extracts `Roo::Excelx::ExceedsMaxError` into a class that inherits from
`Roo::Error`.

The new class is renamed as `Roo::TooManyCellsError` in order to clarify
which maximum is being exceeded.
@@ -112,7 +110,10 @@ def initialize(filename_or_stream, options = {})

if cell_max
cell_count = ::Roo::Utils.num_cells_in_range(sheet_for(options.delete(:sheet)).dimensions)
raise ExceedsMaxError.new("Excel file exceeds cell maximum: #{cell_count} > #{cell_max}") if cell_count > cell_max
if cell_count > cell_max
raise Roo::TooManyCellsError,
Copy link
Contributor

Choose a reason for hiding this comment

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

use fail instead of raise

fail Roo::TooManyCellsError, ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, interesting - I hadn't considered the semantic difference between raise and fail before. I'll update this.

@stevendaniels
Copy link
Contributor

I prefer Roo::ExceedsCellMaxError. That way the relationship between the error and the option, :cell_max is a bit more explicit.

@simonoff Do you think it would be better if this error class had a fixed message?

class Roo::ExceedsCellMaxError < Roo::Error
  def initialize(msg = "The file has exceeded the maximum number of cells allowed")
    super
  end
end

@reshleman
Copy link
Contributor Author

@stevendaniels Thanks for the feedback here.

I renamed this exception because I find ExceedsCellMaxError and the name of the cell_max option to be confusing and somewhat ambiguous.

For someone who hasn't read the source, it's hard to know what "max" is referring to. Is it:

  • a maximum number of cells?
  • the maximum value for a cell?
  • some other cell-related maximum?

Even the documentation above the initialize method is confusing from the user's point of view because it doesn't explain what to use the cell_max param for:

# optional cell_max (int) parameter for early aborting attempts to parse
# enormous documents.

If it were up to me, I'd consider renaming both the exception and the name of the param to be more intention-revealing.

Just my opinion, though. I'm happy to adjust the PR if you'd prefer to keep the existing name.

@reshleman
Copy link
Contributor Author

I also agree that it makes sense for the exception to match the name of the parameter, though.

@stevendaniels
Copy link
Contributor

@reshleman

You make some good points. I agree with you: the names of both the class and the option are a bit ambiguous.

Maybe something like ExceedsMaxCellCountError would be better. It could be paired up with an option called max_cell_count. Obviously I will need to spend more time thinking about an appropriate name for both.

@stevendaniels stevendaniels added this to the v3.0.0 milestone Aug 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants