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

remove pointless comments #2689

Merged
merged 1 commit into from
Nov 7, 2019
Merged

remove pointless comments #2689

merged 1 commit into from
Nov 7, 2019

Conversation

hannesa2
Copy link
Contributor

especially where I'm the last changed user and git blame points to me.

It's just noise

@hannesa2 hannesa2 requested review from davigonz and jesmrec October 30, 2019 21:56
@hannesa2
Copy link
Contributor Author

hannesa2 commented Nov 2, 2019

@michaelstingl It looks like your tests are broken, even removing pointless comments fails.
How can this happen ?

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 4, 2019

@hannesa2 sometimes, the BitRise fails due to some timeouts or performance problems in its internal emulators. We are looking for the best solutions.

@abelgardep
Copy link
Contributor

From my point of view it is important to have gradle files organized. We are working on modularization, so we will have a build gradle file for each module. It is important to have them localized and commented in some way. What do you think @davigonz ?

@hannesa2
Copy link
Contributor Author

hannesa2 commented Nov 4, 2019

From my point of view it is important to have gradle files organized.

You can organise them like you want, but
image
but this comment (and the others) has no purpose, except noise. And worst, git blame shows me, I'm the commiter

@jesmrec jesmrec added this to the 2.14.0 milestone Nov 5, 2019
owncloudApp/build.gradle Outdated Show resolved Hide resolved
owncloudApp/build.gradle Outdated Show resolved Hide resolved
owncloudApp/build.gradle Outdated Show resolved Hide resolved
owncloudApp/build.gradle Show resolved Hide resolved
owncloudApp/build.gradle Show resolved Hide resolved
owncloudApp/build.gradle Show resolved Hide resolved
owncloudApp/build.gradle Show resolved Hide resolved
owncloudApp/build.gradle Show resolved Hide resolved
owncloudApp/build.gradle Outdated Show resolved Hide resolved
owncloudApp/build.gradle Outdated Show resolved Hide resolved
@davigonz
Copy link
Contributor

davigonz commented Nov 6, 2019

@hannesa2 I'm in favor of not writing comments everywhere but providing that the code is self explanatory enough. As I wrote in the code review, some of the comments in the build.gradle can be removed for sure but there are others that would be very useful for someone new in the project who does not necessarily need to know what every dependency is for.

Imagine that someone wants to work on the image loading and goes to the build.gradle file to research about the library being used. It would be impossible to find it without proper comments.

especially where I'm the last changed user and git blame points to me
@hannesa2 hannesa2 force-pushed the RemovePointLessComments branch from 8771e31 to f1052ca Compare November 7, 2019 05:35
@hannesa2
Copy link
Contributor Author

hannesa2 commented Nov 7, 2019

Now I keep a lot of pointless comments. -sad-

It's always a question for whom you write comments. For pros or beginners. There are some who always scream for more documentation, but this ones you will never satisfy, because they have other issues

Documentation is like a newspaper: When it's printed, it's outdated. After some time documentation is just garbage

@davigonz
Copy link
Contributor

davigonz commented Nov 7, 2019

@hannesa2

It's always a question for whom you write comments. For pros or beginners.

I would say that comments should not explain everything from scratch, e.g. people who don't know what is an AndroidManifest.xml, it does not make any sense to explain with comments what is the AndroidManifest used for and as you say, they have many other issues that can be solved by reading/practicing more Android development.

In our case, this is an open source project and it's very common to have beginners developers. In deed, I have had to explain many things from scratch to some people and is not a problem for me.

About this PR, I think that some comments that you wanted to remove from build.gradle are not only for beginners, someone with much experience might not know that glide library is used for image loading for example.

Documentation is like a newspaper: When it's printed, it's outdated. After some time documentation is just garbage

I do not fully agree with you, it's true that the documentation ends up being outdated but is no excuse for not writing documentation or putting some comments when needed. A comment from time ago can be useful for me now and we also read Android documentation (new and a bit old) very often as well 😄

@davigonz davigonz closed this Nov 7, 2019
@davigonz davigonz reopened this Nov 7, 2019
@hannesa2
Copy link
Contributor Author

hannesa2 commented Nov 7, 2019

Let's stop this senseless discussion. That means you keep my name on this pointless comment during git blame

@hannesa2 hannesa2 closed this Nov 7, 2019
@davigonz davigonz reopened this Nov 7, 2019
@davigonz
Copy link
Contributor

davigonz commented Nov 7, 2019

I don't understand the reason for closing this PR now, when you have already applied the changes I requested yesterday.

The code is approved, @jesmrec no QA needed, I will merge it when all the checks are green

@davigonz davigonz merged commit 8e6a59b into master Nov 7, 2019
@delete-merged-branch delete-merged-branch bot deleted the RemovePointLessComments branch November 7, 2019 09:40
@davigonz davigonz added the Sprint label Nov 7, 2019
@hannesa2 hannesa2 mentioned this pull request Nov 9, 2019
@jesmrec jesmrec removed the Sprint label Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants