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

Generate a shorter version of registration.php #280

Merged
merged 1 commit into from Mar 30, 2017

Conversation

Projects
None yet
2 participants
@Zifius
Contributor

Zifius commented Mar 29, 2017

Magerun pull-request check-list:

  • Pull request against develop branch (if not, just close and create a new one against it)
  • README.md reflects changes (if any)

Subject: Generate a shorter version of registration.php

Changes proposed in this pull request:

  • Made registration.php a bit shorter and more readable by use-ing namespace
@tkn98

This comment has been minimized.

Show comment
Hide comment
@tkn98

tkn98 Mar 30, 2017

Contributor

Thanks for your PR. We have a guidleine to terminate all lines in text-files with the newline marker. This includes the last line. This conflicts with the first commit in the PR which changes diverse files to no newlines at the end of file. So these are not extra ending lines but just newlines at the end of file, which is intended and not considered an error (compare with 3.206 Line in POSIX/IEEE Std 1003.1-2008, 2016 Edition).

The other change is correct, the use clause is superfluous and should be removed.

If you may update the PR by removing 368e356 and keeping 2b7e3b2? That would be great for a straight forward merge.

Contributor

tkn98 commented Mar 30, 2017

Thanks for your PR. We have a guidleine to terminate all lines in text-files with the newline marker. This includes the last line. This conflicts with the first commit in the PR which changes diverse files to no newlines at the end of file. So these are not extra ending lines but just newlines at the end of file, which is intended and not considered an error (compare with 3.206 Line in POSIX/IEEE Std 1003.1-2008, 2016 Edition).

The other change is correct, the use clause is superfluous and should be removed.

If you may update the PR by removing 368e356 and keeping 2b7e3b2? That would be great for a straight forward merge.

@Zifius

This comment has been minimized.

Show comment
Hide comment
@Zifius

Zifius Mar 30, 2017

Contributor

@tkn98 Thanks for the feedback Tom, should the xml files also contain a new line in the end of files? Currently only module.xml.twig has it while other files in etc folder do not

The other change is correct, the use clause is superfluous and should be removed.

Not sure I follow, I'm actually adding the use statement here to avoid using FQ namespace twice

Contributor

Zifius commented Mar 30, 2017

@tkn98 Thanks for the feedback Tom, should the xml files also contain a new line in the end of files? Currently only module.xml.twig has it while other files in etc folder do not

The other change is correct, the use clause is superfluous and should be removed.

Not sure I follow, I'm actually adding the use statement here to avoid using FQ namespace twice

@tkn98

This comment has been minimized.

Show comment
Hide comment
@tkn98

tkn98 Mar 30, 2017

Contributor

@Zifius: You're totally right, I meant the "as" of the use clause which is superfluous, not the use clause itself as I wrote.

Can you ammend the PR and remove the first and last commit (the in + revert) and then force-push? That would be great then I can merge it directly, otherwise I would need to squash my own.

Contributor

tkn98 commented Mar 30, 2017

@Zifius: You're totally right, I meant the "as" of the use clause which is superfluous, not the use clause itself as I wrote.

Can you ammend the PR and remove the first and last commit (the in + revert) and then force-push? That would be great then I can merge it directly, otherwise I would need to squash my own.

@Zifius

This comment has been minimized.

Show comment
Hide comment
@Zifius

Zifius Mar 30, 2017

Contributor

@tkn98 Sure thing, but please give me feedback on the etc/*.xml files so that I don't push more commits after squashing. My current intension is to remove new line in EOF in module.xml.twig

Contributor

Zifius commented Mar 30, 2017

@tkn98 Sure thing, but please give me feedback on the etc/*.xml files so that I don't push more commits after squashing. My current intension is to remove new line in EOF in module.xml.twig

@tkn98

This comment has been minimized.

Show comment
Hide comment
@tkn98

tkn98 Mar 30, 2017

Contributor

Yes XML files are also of the text type and should contain these, too.

Btw. some more practical background about these newline characters: this allows to put multiple files after each other into a stream w/o loosing the information where the last line ended. This makes such files much more compatible with various text-tools.

Sorry that this was not so clear upfront that we have this for all text-files.

Contributor

tkn98 commented Mar 30, 2017

Yes XML files are also of the text type and should contain these, too.

Btw. some more practical background about these newline characters: this allows to put multiple files after each other into a stream w/o loosing the information where the last line ended. This makes such files much more compatible with various text-tools.

Sorry that this was not so clear upfront that we have this for all text-files.

@Zifius

This comment has been minimized.

Show comment
Hide comment
@Zifius

Zifius Mar 30, 2017

Contributor

Thanks @tkn98, I squashed the commits and added some missing new lines. Ready for your review

Contributor

Zifius commented Mar 30, 2017

Thanks @tkn98, I squashed the commits and added some missing new lines. Ready for your review

@tkn98 tkn98 merged commit 2c7585d into netz98:develop Mar 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tkn98

This comment has been minimized.

Show comment
Hide comment
@tkn98

tkn98 Mar 30, 2017

Contributor

Thanks!

Contributor

tkn98 commented Mar 30, 2017

Thanks!

@Zifius Zifius deleted the Zifius:feature/registration-php-cleanup branch Mar 30, 2017

@Zifius

This comment has been minimized.

Show comment
Hide comment
@Zifius

Zifius Mar 30, 2017

Contributor

Cheers @tkn98!

Contributor

Zifius commented Mar 30, 2017

Cheers @tkn98!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment