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: Remove trailing spaces #1627

Merged
merged 1 commit into from
Jan 3, 2020
Merged

fix: Remove trailing spaces #1627

merged 1 commit into from
Jan 3, 2020

Conversation

ffflorian
Copy link
Contributor

@ffflorian ffflorian commented Dec 22, 2019

This PR will remove trailing spaces in all files. Best viewed without whitespace changes.

Copy link
Member

@NobbZ NobbZ left a comment

Choose a reason for hiding this comment

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

Also in my opinion, canonical data hasn't changed, we should find a way to get in the whitespace repaired version without bumping...

||_||_|
||_| _|

Copy link
Member

@NobbZ NobbZ Dec 22, 2019

Choose a reason for hiding this comment

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

The whitespace in this code block is significant and not allowed to be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about changing it to this?

    _  _  #
  | _| _| #
  ||_  _| #
          # (blank fourth row)

Copy link
Member

Choose a reason for hiding this comment

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

With literal double crosses?

I think that would lead to similar discussion we had on another exercise where a border was drawn in the examples for visual clarification but not in the test suits for easier programmatic access... I think it was the minesweeper exercise.

Just let the whitespace where it belongs and don't add arbitrary characters which could be misinterpreted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. I reverted the changes to this file.

@rpottsoh
Copy link
Member

rpottsoh commented Dec 22, 2019

I have updated the initial comment. These changes may now be merged without the requirement to bump versions. The version bumps should be reversed.

@rpottsoh
Copy link
Member

@ffflorian please include the statement I have confirmed that no JSON version change is needed as part of the comment in your next commit. I am not sure that where I have inserted it will make any difference. The override message will work if it is in a commit message.

@ffflorian
Copy link
Contributor Author

ffflorian commented Dec 23, 2019

@rpottsoh

@ffflorian please include the statement I have confirmed that no JSON version change is needed as part of the comment in your next commit.

Done, it works :) Just waiting for an answer from @NobbZ.

I have confirmed that no JSON version change is needed
@rpottsoh
Copy link
Member

rpottsoh commented Jan 2, 2020

Happy New Year @NobbZ! I think @ffflorian has addressed both of the issues we had. What do you think?

@rpottsoh rpottsoh merged commit 63bb34c into exercism:master Jan 3, 2020
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