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

Don't use pseudoterminal #323

Closed
wants to merge 1 commit into from
Closed

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Mar 14, 2020

I'm not sure why we use pseudoterminals, so here's a PR to remove them and see what breaks :-)

@codecov
Copy link

codecov bot commented Mar 14, 2020

Codecov Report

Merging #323 into master will increase coverage by 0.51%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
+ Coverage   79.61%   80.13%   +0.51%     
==========================================
  Files          55       55              
  Lines        3209     3181      -28     
  Branches      534      522      -12     
==========================================
- Hits         2555     2549       -6     
+ Misses        607      593      -14     
+ Partials       47       39       -8
Impacted Files Coverage Δ
colcon_core/task/__init__.py 98.98% <ø> (ø) ⬆️
colcon_core/subprocess.py 79.59% <100%> (+12.92%) ⬆️

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 af7fa08...4078610. Read the comment docs.

@rotu rotu changed the title WIP: Don't use pseudoterminal Don't use pseudoterminal Mar 24, 2020
@rotu
Copy link
Contributor Author

rotu commented Mar 24, 2020

@dirk-thomas seeing as all tests pass, is there a reason this is a bad idea?

@dirk-thomas
Copy link
Member

dirk-thomas commented Mar 24, 2020

Until 2h ago the PR was markes as WIP so I didn't look at it until now because of that.

I will review the PR once I have time to do so. At the moment there are several other tasks with higher priority though.

@rotu
Copy link
Contributor Author

rotu commented Mar 24, 2020

Absolutely :-) I had it marked as WIP since I was sure I was wrong. There must have been some reason to do it "the hard way", but after digging, I couldn't figure out why.

@dirk-thomas
Copy link
Member

Please see the reply to your original question: colcon/colcon-cmake#67 (comment)

Since this change breaks functionality I will go ahead and close the PR as invalid.

@dirk-thomas dirk-thomas added the invalid This doesn't seem right label Mar 25, 2020
@dirk-thomas
Copy link
Member

See colcon/colcon-cmake#67 (comment) for a proposal for a backward compatible transition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Development

Successfully merging this pull request may close these issues.

None yet

2 participants