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

Fix a bug that was breaking shell-strip-env #11

Merged
merged 1 commit into from
Nov 9, 2017

Conversation

monomon
Copy link
Contributor

@monomon monomon commented Nov 9, 2017

  • assume passed buffer name would be first arg in &rest
  • Fix activate on windows - do not use "source", and add a newline to
    the command
  • Replace an ad-do-it with (apply orig-fun...) - seems like advice-add
    doesn't provide ad-do-it?

name would be first arg in &rest

* Fix activate on windows - do not use "source", and add a newline to
  the command
* Replace an ad-do-it with (apply orig-fun...) - seems like advice-add
  doesn't provide ad-do-it?
@necaris
Copy link
Owner

necaris commented Nov 9, 2017

@monomon Thank you so much for this PR! I've been trying to figure out how best to replicate #10 -- I don't use Windows, and honestly don't use eshell much, so this hadn't bit me before.

Elisp code looks good -- Indeed, advice-add doesn't provide ad-do-it, that's the legacy function-advice model that I copied over from virtualenv.el and missed updating when I moved over to advice-add... 😥

Looking at other best-practices, it seems sending a newline after the activate command doesn't hurt on any OS, so it might be worth factoring that out into a separate call that's sent regardless of the OS. Otherwise, this looks great, thank you again @monomon !

@monomon
Copy link
Contributor Author

monomon commented Nov 9, 2017

@necaris thank you for the response. I too do not use Windows voluntarily, but now will be on it extensively for a specific project, so I could do some more testing on the package under Windows.

Seems like the CI is not configured properly? Could I help in this respect?

@JoshOBrien
Copy link

@monomon Thanks! I can confirm that your patch fixes the problem I reported on my Windows 7 machine. I really appreciate your (and @necaris ') contributions.

@necaris
Copy link
Owner

necaris commented Nov 9, 2017

@monomon the CI is failing because there aren't actually any useful tests -- I've got a branch I'm working on to add tests, hoping to have this working as expected this weekend. In the meantime I'll go ahead and merge this PR, looking forward to more bugs you report on Windows!

@necaris necaris merged commit 785df98 into necaris:master Nov 9, 2017
Copy link
Owner

@necaris necaris left a comment

Choose a reason for hiding this comment

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

Beans!

Copy link
Owner

@necaris necaris left a comment

Choose a reason for hiding this comment

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

Beans!

Copy link
Owner

@necaris necaris left a comment

Choose a reason for hiding this comment

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

Beans!

Copy link
Owner

@necaris necaris left a comment

Choose a reason for hiding this comment

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

Warmed-up beans?

Copy link
Owner

@necaris necaris left a comment

Choose a reason for hiding this comment

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

Please warm up the beans.

Copy link
Owner

@necaris necaris left a comment

Choose a reason for hiding this comment

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

Beans look nice and warm, good show!

Copy link
Owner

@necaris necaris left a comment

Choose a reason for hiding this comment

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

Cooled down beans!

Copy link
Owner

@necaris necaris left a comment

Choose a reason for hiding this comment

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

Beans need to cool down

Copy link
Owner

@necaris necaris left a comment

Choose a reason for hiding this comment

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

Let's check the temperature of these beans, OK?

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