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

[minor] Fixed two generics related warnings (seen in Eclipse) #81

Closed
wants to merge 1 commit into from

Conversation

vorburger
Copy link

No description provided.

@Deamon5550
Copy link
Contributor

Could you write this as a comment on #23 rather than a PR, thanks.

@Deamon5550 Deamon5550 closed this Jul 3, 2015
@twizmwazin
Copy link

@Deamon5550 I don't see the reason to close a pull request immediately and tell them to report it as an issue for someone else to fix. That seems silly and redundant.

@ST-DDT
Copy link
Member

ST-DDT commented Jul 3, 2015

@twizmwazin
Every merge requires the team to checkout the fix and test whether it actually does what it should.
For small none bug fixes this is a really heavy time overhead. Consider required rebases and merges for other work they do in parallel
To save the team that time the OCD list exists.

Although i agree with you.
I also whished i could fix some (checkstyle) warnings in order to check whether i caused some myself.

@twizmwazin
Copy link

@ST-DDT That kinda defeats a large idea behind an open source, community-driven project. Rather than encouraging community members to contribute you point them to an complaint box.
This being a trivial thing, the only thing I feel is really necessary is check if the warning can be fixed rather than suppressed.

@Deamon5550
Copy link
Contributor

@twizmwazin I really don't understand your comment about this not being open-source. If you want to submit a PR for minor issues then by all means submit a PR for minor issues, you can get the entries of the OCD list, the couple hundred checkstyle warnings thrown by gradle, and I'm sure if you just start looking you'll find even more places where the code style could use some cleanup or a comment has bad grammar.

However I'll save you right now and suggest that you do not do this as you will have a nightmare of rebasing and merge conflicts to handle every time someone pushes something to master before your PR is accepted and a nightmare for everyone else having to rebase their branches across your changes once they are in.

This is why we have the minor issue list so that we can track these small fixes and fix them during a freeze of the repository where we don't have to worry about having to rebase anything (usually just before version releases).

@twizmwazin
Copy link

@Deamon5550 I'm not saying that the project isn't open source, as it clearly and very much is. Although a basic concept behind open source is the idea of community contributions to a project, you seem to promote making an issue for a collaborator to fix rather than allow a community member to actually provide a fix. In the case of something as small as this, I would think double checking and merging this would take just about as long as actually writing the fix yourself. But since he has already made the commit and opened the pull request, why not just double check and merge the commit now rather than put it off for someone else to do again down the road?

@Zidane
Copy link
Member

Zidane commented Jul 3, 2015

@twizmwazin

We only do mass fixes for "OCD Problems" during codebase freezes. That way we don't have to rebase someone's PR they did for fixes during times of potentially high commit volume.

Its far easier to dedicate a team member to do one massive fell swoop of this vs. getting a community cleanup pr mergable in the midst of other PRs/contributions.

EDIT: To further my point, this is nothing against the OP or any community member, its pretty standard for formatting/style issues to be declined.

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.

5 participants