Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Fixed regression in handling of imports (fixes #160) #161

Merged
merged 4 commits into from
Mar 28, 2020

Conversation

jameslamb
Copy link
Owner

In this PR, I attempted to fix the issue @austin3dickey raised in #160 . The fix here returns doppel-cli's handling of standard-lib functions (e.g. from warnings import warn) to the previous behavior.

I added @austin3dickey's reproducible example as a test package and added tests on it. Note that in those tests, I'm not taking a stand on what the behavior should be, just trying to document the current behavior so regressions like this aren't accidentally introduced in the future.

For example, right now I know that doppel-cli isn't doing the "right" thing for a package like feather, where it seems obvious that they are trying to "re-export" from other packages

from pyarrow.feather import (read_feather as read_dataframe,  # noqa
                             write_feather as write_dataframe,
                             FeatherError,
                             FeatherReader,
                             FeatherWriter)

As of this commit, runningdoppel-describe -p pythonspecific2 -l python -d out/ yields:

{
  "name": "pythonspecific2 [python]",
  "language": "python",
  "functions": {
    "create_warm_things": {
      "args": [
        "~~KWARGS~~"
      ]
    },
    "create_warning": {
      "args": []
    }
  },
  "classes": {}
}

I tested with doppel-cli==0.2.1 and got the exact same output.

The tests are passing for the other built-in cases I tried to address in #155 so I don't think this is undoing that work.

shmeate_schmarning = warn


def create_warm_things(**kwargs):

Choose a reason for hiding this comment

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

This whole file is great, but this is the best line.

@jameslamb
Copy link
Owner Author

I think this PR is stalled because Travis is struggling (microsoft/LightGBM#2954 (comment))

@jameslamb
Copy link
Owner Author

closing and re-opening to retrigger CI correctly. Travis had an outage today caused by a GCP outage.

@jameslamb jameslamb closed this Mar 27, 2020
@jameslamb jameslamb reopened this Mar 27, 2020
@jameslamb jameslamb closed this Mar 28, 2020
@jameslamb jameslamb reopened this Mar 28, 2020
@codecov-io
Copy link

codecov-io commented Mar 28, 2020

Codecov Report

Merging #161 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #161   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         316    316           
=====================================
  Hits          316    316

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 72c4ad5...eb35eb1. Read the comment docs.

@jameslamb
Copy link
Owner Author

I think the travis outage has left this PR in a weird state. Also they had this incident yesterday:

image

I can clearly see that the build passed: https://travis-ci.org/github/jameslamb/doppel-cli/builds/667965392, so I'm going to use executive power and merge.

@jameslamb jameslamb merged commit 35b2320 into master Mar 28, 2020
@jameslamb jameslamb mentioned this pull request Mar 28, 2020
@jameslamb jameslamb deleted the bugfix/builtins branch June 6, 2020 02:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants