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

Extend BadLocationException by illegal argument information #1752

Closed
wants to merge 2 commits into from

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Mar 13, 2024

As logged for example during
ConsoleDocumentAdapterTests.testInvalidInvocations()

As logged for example during
ConsoleDocumentAdapterTests.testInvalidInvocations()
Copy link
Contributor

github-actions bot commented Mar 13, 2024

Test Results

   918 files  +    1     918 suites  +1   1h 10m 21s ⏱️ + 31m 39s
 7 507 tests ±    0   7 356 ✅ +    3  151 💤 ±  0  0 ❌  - 3 
23 670 runs  +1 573  23 165 ✅ +1 457  505 💤 +119  0 ❌  - 3 

Results for commit 446ee5e. ± Comparison against base commit 75355f9.

♻️ This comment has been updated with latest results.

@jukzi
Copy link
Contributor Author

jukzi commented Mar 13, 2024

The idea & style was copy pasted from org.eclipse.jface.text.ListLineTracker.getLineNumberOfOffset(int). Its good to have the same style all over.
I don't see a value to have complete sentences in Error messages that should not happen anyway. Also they would make the logfiles even longer without additional information.
Such simple messages are also common in JDK, for example java.util.Objects.requireNonNullElseGet(T, Supplier<? extends T>)

If you require your suggested changes please put a -1 review.

@laeubi
Copy link
Contributor

laeubi commented Mar 13, 2024

The idea & style was copy pasted from org.eclipse.jface.text.ListLineTracker.getLineNumberOfOffset(int). Its good to have the same style all over.

Its not good to copy bad style.

I don't see a value to have complete sentences in Error messages that should not happen anyway. Also they would make the logfiles even longer without additional information.

I have given already example why I think this is better, if you read the logs you need to also read the source code to knwo what this is about

Such simple messages are also common in JDK, for example java.util.Objects.requireNonNullElseGet(T, Supplier<? extends T>)

And they also supply variant to give a descriptive message or even throw a custom exception in such cases....

If you require your suggested changes please put a -1 review.

I gave some hints how it can be imrpoved but I don't require it, but your intend was to improve things in case of failures so next time you see this message you might be happy to have better messages ... or not, at laest all this arguing why the cryptic one is better takes more time than simply chosse a good one.

@jukzi
Copy link
Contributor Author

jukzi commented Mar 13, 2024

My only desire here is to get the illegal argument values into the logs. I have no plans to improve error messages that a user should nor see.

@akurtakov
Copy link
Member

That's ENOUGH!
I strongly SUGGEST that everyone learns to read everyone else's message as an effort to get a better software at the end!
This behavior of "everyone knows better than everyone else should stop RIGHT NOW" .
Which one is better doesn't really matter here as the discussions tone degrade on daily basis to unacceptable one.

@merks
Copy link
Contributor

merks commented Mar 13, 2024

I sometimes get the feeling that folks are more willing to argue an hour over the tiniest nit than they are willing to spend five minutes making the code arguably better. There is something seriously wrong with that behavior and no matter how many times I keep asking for better communication, the poor communication just happens again and again. Something needs to improve immediately or the PMC will step in and take action, and those are actions that people are very likely not to like.

So here I see @jukzi improving something; that's wonderful. I see @laeubi comes with constructive suggestion for further improvements; that's helpful and also great teamwork. But then it degrades. Yes, @laeubi suggestion has grammar errors it and it omits to include the value itself in the one of the messages, but the intent of his suggestion is clear and one would be well advised to listen.

I'm so fed up with poor communication, especially after yesterday where I had to deal with communication of this form:

From past experience, "feel free to do as you think best" means go away, waste your own time and I'll bin the results.

where someone reads what I write and believes that I literally I mean the opposite of what I actually said. It's impossible to communicate when ill intent is assumed.

So I know all too well that it's really a challenge to remain civil in the face of frustration, but it is what we must do. I'll bet the mind space here sometimes more like "you nitpicked my last changes to death so I'm going to nitpick your latest changes to death", and even when that's not actually the intent, if one believes it to be the intent, one reacts as if the other person has ill intent. But many of us are super picky and that can be super annoying, but it's also what makes the final results super good.

@jukzi
Copy link
Contributor Author

jukzi commented Mar 13, 2024

If somebody wants to further improve the error messages feel free to override this PR.
I will not merge this PR myself in the current social state.

@merks
Copy link
Contributor

merks commented Mar 13, 2024

@jukzi

I would like to have a private one-on-one conversation with you. I'm swamped today with SimRel, but the rest of the week is good. How best to arrange that? I think you know my gmail address. I am also on IDE WG slack channel...

@jukzi
Copy link
Contributor Author

jukzi commented Mar 13, 2024

I would like to have a private one-on-one conversation with you.

Appreciated. Just call +49 151 18233332 during office hours. Anyone else interested is invited too.

@jukzi jukzi closed this Apr 4, 2024
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