search

eclipse-platform / eclipse.platform.resources

Eclipse Public License 2.0
3 stars 18 forks source link

Revert "Revert "Improve IFile.getLineSeparator() API"" #161

Closed mickaelistria closed 2 years ago

mickaelistria commented 2 years ago

This reverts commit 166a3554857885f0ae81cff2f739847e08c9b1fa.

jukzi commented 2 years ago

please discuss you API before you rush it in.

mickaelistria commented 2 years ago

I discussed the API in #159. Please read and learn from it.

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-161/1/

jukzi commented 2 years ago

Please read and learn from it.

how to not do it? Do you plan to encourage committers and contributors or to disgust them?

mickaelistria commented 2 years ago

I wrote https://github.com/eclipse-platform/eclipse.platform.resources/pull/159#issuecomment-1154092618 which explains why the change is better for the project. It relies on existing practices and OO patterns that have been documented in letterature for a long time and have successfully power Eclipse Platform so far.

Do you plan to encourage committers and contributors or to disgust them?

I plan to encourage them in building high-quality software and that involves polished and consistent APIs. I offered my time to take your API proposal and polish it. What's the actual issue with you? Just the fact that I changed your code?

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-161/2/

jukzi commented 2 years ago

Mickael, you explained why you think it its better. but no one agreed. i am open to discuss the API and do not bother if my code is being changed. the only thing that bothers me is that the new contributor in https://github.com/eclipse-platform/eclipse.platform.debug/pull/28 will be not happy to have extra work and that is not good to build community. just get his change finished and then you can take the extra work to "polish" it.

mickaelistria commented 2 years ago

Mickael, you explained why you think it its better. but no one agreed.

I have experience with that, and mind you, I was in your position some years ago, and I learnt from it and from my reviewers. My opinion is backed by solid OO patterns, documented in multiple books, and experience built by maintenance of the resource model by many people. Mimicking what's working or following state of art rule is not something that I thought would require such a debate.

Going fast can often the enemy of going well. And even if it helps a contributor in implementing X or Y, APIs are a too critical point to do things too fast at the risk of being inconsistent or fragile. Reverting an API addition is extremely hard to do well as the impact on consumers is huge and can be very harmful; on the other end keeping incorrect APIs is something that reduces maintenance productivity and quality and capability to innovate for a long time. Maybe you don't have a sense of how difficult API maintenance is for Platform, of how much they have a negative impact on innovation after a few years, of how much work it is to undo things wrong -when it is ever possible-; but trust me here, it's huge, it's the worst. Saving 1 day to a contributor is not worth the risk; the criticity of APIs is worth bugging contributor a bit more (or better, offering them to fill the gaps). Independently of the context, APIs need a lot of care and this cause delay. Reviews are fundamental, and -all due respect- require a lot discussion and expertise(s from several people) to achieve something sustainable. As a project lead, it's my duty to preserve the interests of the project on the long term, and my experience (which is also the experience previous leads have shared with me) drive me to think that incorrect APIs have to be killed in the egg, even if it delays another patch.

jukzi commented 2 years ago

I won't continue to discuss your opinion. Instead i am leaving into extended weekend and wish you a pleasant public holiday (assuming/hoping that you got that too tomorrow).

eclipse-releng-bot commented 2 years ago

The Jenkins build of this PR has now completed. See details at https://ci.eclipse.org/platform/job/eclipse.platform.resources/job/PR-161/3/

akurtakov commented 2 years ago

Reasoning here is sound thus merging. Please open new issue with good explanation what would the benefit be of another approach.

ghost commented 2 years ago

@akurtakov technical speaking you just committed a revert which did not resolve breakage which yourself stated SHOULD NOT happen Also you submitted a change with active disagreement without conflict resolution and without stating your position.

I ask yourself to revert it. Otherwise please take the necessary steps to guide the depending contributor to repair his PR. I'm out.

akurtakov commented 2 years ago

You got me, sorry about that. As every effort to streamline processes it will take time for everyone to get used to it and follow the steps including me . Note that as everything else here the guidelines are open for improvement too (request PMC attention to the issue) . With that said: I did that as as PMC and trying to get things to initial state so real discussion starts from the state prior of "reverts" show .