Skip to content

Conversation

cslzchen
Copy link
Contributor

@cslzchen cslzchen commented Apr 2, 2018

Ticket

SVCS-677

Purpose

Fixed the bug where unsupported export types throw 400 assertion error instead of 400 exporter error. Remove the misleading comment which wrongly claims that get_exporter_name() is always called with a supported exporter. Update tests accordingly.

Changes

Before

before

After

after

Side effects

No

QA Notes

TBD

Deployment Notes

No

Copy link
Contributor Author

@cslzchen cslzchen left a comment

Choose a reason for hiding this comment

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

Fixed as we discussed @felliott . Ready for CR 🎆 .


# `make_renderer()` is called before `make_exporter()` to ensure the file type is supported
# Empty list indicates unsupported export type. Return '' and let `make_exporter()` handle it.
if len(ep_list) == 0:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix follows exactly the same code used in get_renderer_name(). In addition, the comment is corrected.

@coveralls
Copy link

coveralls commented Apr 2, 2018

Coverage Status

Coverage decreased (-0.2%) to 71.254% when pulling ebc764e on cslzchen:bug-fix/unsupported-exporter into 68ede65 on CenterForOpenScience:master.

@felliott felliott changed the title [HotFix] [SVCS-???] Fix Unsupported Export Type [HotFix] [SVCS-677] Fix Unsupported Export Type Apr 2, 2018
Copy link
Member

@felliott felliott left a comment

Choose a reason for hiding this comment

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

If the exporter name is the empty string, the cache filename will end with a period. I think if its blank, we shouldn't add the period at all.

@cslzchen
Copy link
Contributor Author

cslzchen commented Apr 2, 2018

@felliott I updated both cases for render.py and exporter.py. Ready for CR again.

@cslzchen cslzchen force-pushed the bug-fix/unsupported-exporter branch 2 times, most recently from 5155c22 to 249115d Compare April 4, 2018 17:59
cslzchen added 2 commits April 4, 2018 14:01
Fixed the bug where unsupported export types throw 400 assertion error instead of 400 exporter error. Remove the misleading comment which wrongly claims that `get_exporter_name()` is always called with a supported exporter. Update tests accordingly.
- R/E: renderer name and exporter name - `get_render_name()` and `get_exporter_name()` return '' when rendering/exporting a given type is not supported. Update `cache_file_path` for both to be removed of the trailing dot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants