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 six library #1058

Merged
merged 5 commits into from
Mar 1, 2022
Merged

Conversation

Leenkiz
Copy link
Contributor

@Leenkiz Leenkiz commented Feb 27, 2022

No description provided.

@datapythonista
Copy link
Member

Thanks for the contribution @Leenkiz. Looks like you've got conflicts, can you please merge master into your branch, resolve the conflicts, and push.. again. We need this to be able to run the CI checks and merge. Thanks!

@dorothykiz1
Copy link
Contributor

Thanks for this @Leenkiz the conflicts mean that the upstream has new changes that your local copy doesn't have so you need to run git fetch upstream and
git merge upstream/master then work on your changes and push.

@dorothykiz1
Copy link
Contributor

Thanks @Leenkiz ,one more change,

import socketserver will solve your issue as socketserver is used but not defined

@datapythonista
Copy link
Member

@dorothykiz1 @LucyJimenez, looks like there are test failures in appveyor that are not showing in GitHub actions. We should have a look and see why is this happening. Do you mind creating an issue, linking to both the appveyor and GitHub actions builds in this PR (not the PR itself, since it'll get lost when the test is fixed), and then have a look and find what's the problem.

In the CI logs you should find the commands that have been executed in both systems. You should be able to implement the bug causing the test failure locally, run both commands, and see one failing and the other passing.

@dorothykiz1
Copy link
Contributor

@dorothykiz1 @LucyJimenez, looks like there are test failures in appveyor that are not showing in GitHub actions. We should have a look and see why is this happening. Do you mind creating an issue, linking to both the appveyor and GitHub actions builds in this PR (not the PR itself, since it'll get lost when the test is fixed), and then have a look and find what's the problem.

In the CI logs you should find the commands that have been executed in both systems. You should be able to implement the bug causing the test failure locally, run both commands, and see one failing and the other passing.

I have created issue #1060 to reference this

@@ -84,9 +83,9 @@ def run_from_conf_args(cls, conf, args):
def run(cls, conf, port=0, browser=False):
os.chdir(conf.html_dir)

class Handler(SimpleHTTPServer.SimpleHTTPRequestHandler):
class Handler(http.server.SimpleHTTPRequestHandler):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like http.server is not defined. We should import it.

@dorothykiz1 looks like we don't have tests for preview. It'd be good to create an issue for it. And also, if you want to have a look at other commands that are not tested, that would be very helpful. Not having any test for a command means that like here, even a bug like this one can be added unnoticed (this would fail as soon as the command is executed, for all cases).

Copy link
Contributor

@dorothykiz1 dorothykiz1 Mar 1, 2022

Choose a reason for hiding this comment

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

Thanks @datapythonista i noticed locally the test cases were being skipped for the GitHub actions CI, I will take a look at this

@datapythonista datapythonista merged commit 44dcbf4 into airspeed-velocity:master Mar 1, 2022
@datapythonista
Copy link
Member

Thanks @Leenkiz, nice clean up. I reverted the part that was tricky, so we could merge the other changes. But feel free to continue in a new PR if you want.

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.

3 participants