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

Validate on all pod commands that the user is using a terminal in utf-8 #1570

Closed
wants to merge 5 commits into from
Closed

Validate on all pod commands that the user is using a terminal in utf-8 #1570

wants to merge 5 commits into from

Conversation

joshkalpin
Copy link
Member

When finished this should fix #1542. I'm not sure if this is the best way to handle this and what to do with the 1.8.7 case but I think it's a start at least.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c9e08db on Kapin:utf-8-validation into 525180c on CocoaPods:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 0cfcf4c on Kapin:utf-8-validation into 525180c on CocoaPods:master.

@fabiopelosin
Copy link
Member

Great!

Some questions:

  • Why did you remove the parenthesis from the if statement? CocoaPods always use them.
  • Should we raise or just print a message in yellow? Not sure about this, just proposing the alternative.
  • I think that we should introduce a little explanation about hot to fix the issue. We could either indicate how to set up the environment directly or link to a relevant resource (for now just an issue, in future maybe a troubleshooting section guides.cocoapods.org).

@joshkalpin
Copy link
Member Author

  1. Removed parenthesis because I was't thinking. I'll restore them.
  2. Raising seems to explode the builder at this point (might be a travis config issue?), but besides that I think that if a user can't use CocoaPods properly without it, I would say it is an error.
  3. The copy was temporary. I'd definitely rather do that :)

@alloy
Copy link
Member

alloy commented Nov 11, 2013

Why did you remove the parenthesis from the if statement? CocoaPods always use them.

Is that since the introduction of the new style tool? Although a quick look through the sources does not seem to indicate they are being used in any if statement.

@fabiopelosin
Copy link
Member

@alloy Actually you're right. I prefer to use the parenthesis though when there is more than one condition tested (i.e. a logic operator is used like &&)

@fabiopelosin
Copy link
Member

Btw, @kapin don't forget about the changelog 😄

@joshkalpin
Copy link
Member Author

@irrationalfab of course. Could you help me look into why travis is breaking? It appears that one of the builders is not using utf-8 encoding :)

@fabiopelosin
Copy link
Member

@kapin ah, it looks like your deduction is correct. You could further investigate it with a test commit :-/ it by printing the value of Encoding.default_external. This is one of the reasons why I'm tempted not to raise. CocoaPods works fine in the 99% even without that setting. The issue manifests itself only with certain Pods.

@joshkalpin
Copy link
Member Author

@irrationalfab, fair enough on that point. I'll convert it to a warning.

@fabiopelosin
Copy link
Member

👍

Updated copy as well to be a little bit more useful.
@joshkalpin
Copy link
Member Author

Wasn't sure the best way of doing the yellow so I improvised. I know we do it elsewhere but I didn't want to start pulling in dependencies all over the place. Once again copy is up for debate but it is better than last time.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5105f98 on Kapin:utf-8-validation into aa25ac0 on CocoaPods:master.

@fabiopelosin
Copy link
Member

@kapin In this specific case, as it would not be appropriate to import dependencies, as you say, I use a solution similar to yours. However instead of monkey patching the string class (with the same method that later on will be overridden by the colored gem) I would prefer to simply wrap the two strings with the color codes inline. I.e.

puts "\e[33mWARNING: blah blah.\e[0m"

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ad7630d on Kapin:utf-8-validation into aa25ac0 on CocoaPods:master.

@fabiopelosin
Copy link
Member

Aceness looks great to me!

@joshkalpin
Copy link
Member Author

@irrationalfab merge as is or would you prefer I squash?

@fabiopelosin
Copy link
Member

As a general rule we prioritize getting things done on keeping a clean history. However in this case I think that a nice squash would be sweet.

@joshkalpin
Copy link
Member Author

@irrationalfab closing this and going to open a squashed one and merge it.

@fabiopelosin
Copy link
Member

@kapin Actually I was wrong :-/ the recommendation, which sounds reasonable to me, from the Ruby Style Guide is:

Don't use parentheses around the condition of an if/unless/while/until.

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.

Warn about UTF support in the environment
4 participants