Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 57.39%. Comparing base (52dc967) to head (5ef9e1d).
Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #6497      +/-   ##
==========================================
- Coverage   57.40%   57.39%   -0.01%     
==========================================
  Files         507      507              
  Lines       31526    31528       +2     
  Branches     3548     3549       +1     
==========================================
  Hits        18096    18096              
- Misses      12582    12583       +1     
- Partials      848      849       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Should we remove it or raise an exception? cc @gramalingam

Should we remove it or raise an exception? cc @gramalingam

I moved the check to convert_model_to_external_data.
Let me know if you think we should overwrite it instead.

Could you add the Raises section in the docstring to describe this new behavior. Thanks! ref: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

Overwriting can be unsafe if the model proto has reference to the existing data file. Maybe we can provide an option? But I also don't prefer overcrowding the options we have. hmm

Could you add the Raises section in the docstring to describe this new behavior. Thanks! ref: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

Thank you @justinchuby, I added the explanations.

Deferring to @gramalingam

@gramalingam any thoughts?

I think it is safer not to overwrite it silently. I am fine with using a user option to overwrite or leaving in the PR's current form (with an exception).