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

Remove unused functions #295

Merged
merged 4 commits into from
Apr 22, 2020
Merged

Remove unused functions #295

merged 4 commits into from
Apr 22, 2020

Conversation

tom-tan
Copy link
Member

@tom-tan tom-tan commented Oct 30, 2019

By introducing tree structure exceptions, several functions and several function parameters are no longer used.
This request removes the following.

  • reformat_yaml_exception_message
    • No longer needed because the exceptions from ruamel parser are wrapped with SchemaSaladException
  • regex_chunk, chunk_messages, indent, bullets, strip_dup_lineno in sourceline.py
    • They are replaced with strip_duplicated_lineno, reflow_all, and SchemaSaladException#pretty_str
  • SourceLine#__enter__, SourceLine#__exit__, SourceLine#makeError
    • They are replaced with a simple exception handling
  • raise_type and include_traceback from the constructor of SourceLine
    • Now exceptions are thrown from outside of SourceLine

@tom-tan tom-tan requested a review from mr-c October 30, 2019 04:31
@codecov
Copy link

codecov bot commented Oct 30, 2019

Codecov Report

Merging #295 into master will increase coverage by 1.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #295      +/-   ##
==========================================
+ Coverage   68.50%   69.61%   +1.10%     
==========================================
  Files          15       15              
  Lines        2848     2797      -51     
  Branches      776      764      -12     
==========================================
- Hits         1951     1947       -4     
+ Misses        721      674      -47     
  Partials      176      176              
Impacted Files Coverage Δ
schema_salad/sourceline.py 70.89% <ø> (+13.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93cdd36...aea9e3b. Read the comment docs.

mr-c
mr-c previously requested changes Oct 30, 2019
Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

This breaks cwltool so the version number in setup.py needs to be increased

ImportError: cannot import name 'strip_dup_lineno' from 'schema_salad.sourceline' (/home/michael/schema_salad/schema_salad/sourceline.py)

@mr-c
Copy link
Member

mr-c commented Oct 30, 2019

A failure from another part of cwltool is

>               with SourceLine(obj, "location", validate.ValidationException, _logger.isEnabledFor(logging.DEBUG)):
E               TypeError: __init__() takes from 2 to 3 positional arguments but 5 were given

Can this be avoided?

@tom-tan
Copy link
Member Author

tom-tan commented Oct 31, 2019

OK, I will send a PR to cwltool.

@tom-tan
Copy link
Member Author

tom-tan commented Oct 31, 2019

I found that the exceptions in cwltool are not structured...
But I leave it as is because it is out of scope of fixing the issue.

@tom-tan
Copy link
Member Author

tom-tan commented Oct 31, 2019

@mr-c Can I change this request to remove reformat_yaml_exception_message, regex_chunk and chunk_messages?

I found that this request includes two types of unused functions:

  • No longer used both of schema_salad and cwltool: reformat_yaml_exception_message, regex_chunk and chunk_messages

  • schema_salad does not use but cwltool uses:

    • indent, bullets, strip_dup_lineno
    • SourceLine#__enter__, SourceLine#__exit__, and SourceLine#makeError

We can safely remove the former but currently it is difficult to remove the latter because they are mainly used to reformat the exception messages. To remove them, it is better to move the exceptions in cwltool to tree structured exceptions but it needs more work than this request want to do.

@mr-c
Copy link
Member

mr-c commented Apr 17, 2020

Can I change this request to remove reformat_yaml_exception_message, regex_chunk and chunk_messages?

yes

@tom-tan
Copy link
Member Author

tom-tan commented Apr 22, 2020

Done! Now this request only removes reformat_yaml_exception_message, regex_chunk and chunk_messages.

@tom-tan tom-tan dismissed mr-c’s stale review April 22, 2020 03:15

I dismiss it because now strip_dup_lineno is not removed.

@mr-c mr-c merged commit fbe70a3 into master Apr 22, 2020
@mr-c mr-c deleted the remove-unused-functions branch April 22, 2020 09:59
@mr-c
Copy link
Member

mr-c commented Apr 22, 2020

Thanks!

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