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 trailing whitespaces #689

Closed
wants to merge 1 commit into from

Conversation

fwininger
Copy link
Collaborator

Fix every trailing whitespaces.

@codecov-io
Copy link

Codecov Report

Merging #689 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
- Coverage   95.71%   95.68%   -0.04%     
==========================================
  Files         144      144              
  Lines        9625     9625              
==========================================
- Hits         9213     9210       -3     
- Misses        412      415       +3
Impacted Files Coverage Δ
test/integration/test_http_proxy.rb 100% <ø> (ø) ⬆️
test/test_buffered_io.rb 100% <ø> (ø) ⬆️
test/integration/test_encoding.rb 100% <ø> (ø) ⬆️
test/integration/test_ed25519_pkeys.rb 100% <ø> (ø) ⬆️
test/start/test_transport.rb 100% <ø> (ø) ⬆️
test/integration/test_cert_user_auth.rb 100% <ø> (ø) ⬆️
test/transport/test_server_version.rb 100% <ø> (ø) ⬆️
test/test_all.rb 100% <ø> (ø) ⬆️
lib/net/ssh/connection/event_loop.rb 46.96% <100%> (ø) ⬆️
lib/net/ssh/test/remote_packet.rb 93.75% <100%> (ø) ⬆️
... and 46 more

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 ce1d320...7d2560f. Read the comment docs.

@fwininger
Copy link
Collaborator Author

@mfazekas can you merge this trival PR ?

@mfazekas
Copy link
Collaborator

@fwininger sorry i don't really like this change. I also don't like the whitespaces at the end of lines.

But this change is a pain in the neck from git standpoint, it makes any merge before the change painfull, and it also makes a lot of noise in git history/annotate, i presonally don't think it's worth it.

@fwininger
Copy link
Collaborator Author

I can only partially agree with you, I like having a clear git history but when the PR changes are mainly on empty lines, I think it's still to good move forward.

@fwininger
Copy link
Collaborator Author

@mfazekas I plan to add modern algorithms to net/ssh : kex curve25519, ciphers aes-gcm, chacha20-poly1305.
For curve25519 see #690, but new ciphers I need to refactor a lot of the source code, because AEAD encryptions don't need mac algorithms.
The current coding style is very crap and I think we really need a update to have a gem with better maintainability with new ruby versions.

Recently I have merged two PR :

So I have two choices :

  • I can fork the project and I do an new gem for the modern usage of ssh and allow other people to contribute easily
  • I can merge progressively all my PR in net/ssh to have a better future maintainability .

Personally, I prefer the second option that follows my phiolosophy of the open source software, but if reject again my PR (see #515) I will choose the first option.

@mfazekas
Copy link
Collaborator

@fwininger Sorry maybe i wasn't clear on reason of #515 rejection. It was rejected as net-ssh is a library that used by many popular apps, which means changes have to be reviewed so they doesn't contain any malware/lines with bad intentions.

Imagine a bad guy with bad intentions opening a PR with rubocop fixes and hiding 1 nasty lines along the trivial changes. See dominictarr/event-stream#116 for example.

I know you don't have bad intentions but as a maintainer i still have to review changes coming from everyone. So you must understand i don't like PR-s with a lot of code changes, esp if they don't improve net-ssh functionality.

I'm very gratefull that you're working on net-ssh improvements. I'd like to ask your understanding and patience regarding huge number of code style changes.

As for this PR, regarding whitespaces as i see it:

  • it's an improvement as it removes unneccesary whitespaces
  • it's a pain as it requires review in a lot of changes
  • it's a pain as it creates conflits in unmerged commits, and complicates git blame/history (we don't have a huge number of commits, so that's also not a huge issue.)

But since it's important to you and you have non trivial PR #690, this is important for me as well. Please give me few days to try/review #690, and this PR then it can be merged, thans for your understanding.

@fwininger
Copy link
Collaborator Author

Thanks for the answer, I understand your point with the need of review (as maintainer myself) and an the security concern (as Pentest myself).

Do you accept a PR with the requirement of ruby 2.3 in place of 2.2 ?

I can give you a tips to review this PR :

git diff HEAD~1 --ignore-all-space --ignore-blank-lines

The output is null so I don't introduice malware code.

@mfazekas
Copy link
Collaborator

Next version will be a major version so sure we can up required ruby version if needed. Some of our devops users, like to be able to support very old ruby versions but 2.3 sounds fine, it's already EOL.

Signed-off-by: Florian Wininger <fw.centrale@gmail.com>
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