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

[AtomFormat] improved to comply with RFC 4287 #995

Merged
merged 10 commits into from
Jan 21, 2019

Conversation

fulmeek
Copy link
Contributor

@fulmeek fulmeek commented Jan 5, 2019

This PR improves the Atom format to comply with RFC 4287. It includes unit tests for verification.

Copy link
Contributor

@logmanoriginal logmanoriginal left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This is a very complex PR, please split it into multiple pieces:

  • Fixing the existing format
  • Extending the format
  • Adding unit tests

That way reviews are easier to do.

Please also don't align code using tabs. It may work in your editor, but it looks awful in mine (because I use different tab width). The GitHub editor for example is also harder to read like this:

image

For the core files (anything except bridges), please use curly braces for if-statements:

if($condition === true) {
    // code here
}

formats/AtomFormat.php Outdated Show resolved Hide resolved
formats/AtomFormat.php Outdated Show resolved Hide resolved
formats/AtomFormat.php Outdated Show resolved Hide resolved
formats/AtomFormat.php Outdated Show resolved Hide resolved
formats/AtomFormat.php Show resolved Hide resolved
formats/AtomFormat.php Outdated Show resolved Hide resolved
@fulmeek
Copy link
Contributor Author

fulmeek commented Jan 9, 2019

Thanks for your suggestions, please see the changes.

@fulmeek
Copy link
Contributor Author

fulmeek commented Jan 11, 2019

Commit 71b5ed4 should fix issue #1004.

Copy link
Contributor Author

@fulmeek fulmeek left a comment

Choose a reason for hiding this comment

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

Thanks again for your suggestions, please see the changes.

@logmanoriginal
Copy link
Contributor

logmanoriginal commented Jan 15, 2019

Thanks, one final thing before I can merge. The alignment tabs need to be removed.

Here is another example from my editor (using a tab width of 4):

image

Here is tab width of 2 for comparison:

image

I'm pretty sure it looks fine in your editor, but there is no way to make alignments work for everyone, so we should avoid using them.

@fulmeek
Copy link
Contributor Author

fulmeek commented Jan 15, 2019

This should do it.

@logmanoriginal logmanoriginal merged commit ab2e566 into RSS-Bridge:master Jan 21, 2019
@logmanoriginal
Copy link
Contributor

Merged. Thanks for the updates!

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.

2 participants