Fix property combination logic #10

Merged
merged 1 commit into from Nov 11, 2016

Projects

None yet

3 participants

@rhinoduck
Contributor

From the commit message:

When both the 'trim_trailing_whitespace' and the 'insert_final_newline'
properties were specified for a section, only the first one in the list
was picked up. Both will be picked up now.

In search for related issues, I only found editorconfig/editorconfig#149, which deals with insert_final_newline not working, but for a different reason. I am mentioning it here, because it should probably be closed as there should not be any more problems with insert_final_newline now.

@rhinoduck rhinoduck Fix property combination logic
When both the 'trim_trailing_whitespace' and the 'insert_final_newline'
properties were specified for a section, only the first one in the list
was picked up. Both will be picked up now.
0619c90
@xuhdev xuhdev self-assigned this Nov 2, 2016
@ffes
ffes approved these changes Nov 6, 2016 View changes

First of all, don't know if am I the person to approve this. But since I'm the one how added the insert_final_newline feature I thought I give my opinion.

Apart from the styling difference with the rest of the code (curly braces when there is only one line of code) looks good to me.

@rhinoduck
Contributor

I can remove the braces if you want, as well as make other style or naming changes. The new logic itself is working fine for me in a custom build of the plugin I now use with npp; I tried testing other paths than just my use case too, and there did not seem to be any problems.

As "Allow edits from maintainers" is enabled for this pull request, I think you should also be able to add the style edits as new commits yourselves, and then squash everything back into one commit as you are merging the PR. I've never done this myself though, so I may be mistaken.

@xuhdev
Member
xuhdev commented Nov 7, 2016

Thank you for your contribution! @ffes It's not a problem at all! Feel free to review. As long as you feel comfortable, we can merge it, as I barely have access to Windows.

@xuhdev
Member
xuhdev commented Nov 10, 2016

@ffes Shall we merge?

@ffes
Member
ffes commented Nov 10, 2016

Merging is fine by me.

BTW, did you see the e-mail I sent you?

@ffes ffes merged commit 294a019 into editorconfig:master Nov 11, 2016
@ffes
Member
ffes commented Nov 11, 2016

Thanks for your contribution

@rhinoduck
Contributor

Glad I could give something back.

@rhinoduck rhinoduck deleted the rhinoduck:property-combination-fix branch Nov 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment