Skip to content

Comments

Continue #2027: throw CoreException instead of returning null#2037

Merged
laeubi merged 1 commit intoeclipse-platform:masterfrom
ruspl-afed:terminalconsoleviewmanager_optional
Jul 14, 2025
Merged

Continue #2027: throw CoreException instead of returning null#2037
laeubi merged 1 commit intoeclipse-platform:masterfrom
ruspl-afed:terminalconsoleviewmanager_optional

Conversation

@ruspl-afed
Copy link
Contributor Author

@laeubi here is the last part of what was suggested for #2027
I plan to work more on non-API part of these types (to avoid double calls for getActiveWorkbenchPage() in some API chains for example)

@github-actions
Copy link
Contributor

github-actions bot commented Jul 13, 2025

Test Results

 1 920 files   -  27   1 920 suites   - 27   1h 31m 12s ⏱️ + 3m 6s
 4 718 tests ±  0   4 693 ✅  -   1   24 💤 ±0  1 ❌ +1 
14 001 runs   - 153  13 833 ✅  - 154  167 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit b8c6c8b. ± Comparison against base commit d4985f2.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@laeubi laeubi 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 working on this, I think instead of using Optionals here it would be much more appropriate to throw a CoreException if something goes wrong as we do elsewhere in Platform code and otherwise never return null.

@ruspl-afed
Copy link
Contributor Author

Thank you for your review @laeubi
I support your vision regarding this kind of signatures and we have much more other signatures to be changed this way in further PRs.

@ruspl-afed ruspl-afed force-pushed the terminalconsoleviewmanager_optional branch 2 times, most recently from a83024c to c1e2daf Compare July 14, 2025 09:54
@ruspl-afed ruspl-afed changed the title Continue #2027: return Optional instead of null Continue #2027: throw CoreException instead of returning null Jul 14, 2025
@ruspl-afed ruspl-afed force-pushed the terminalconsoleviewmanager_optional branch from c1e2daf to f9c8cdb Compare July 14, 2025 10:48
@ruspl-afed ruspl-afed force-pushed the terminalconsoleviewmanager_optional branch from f9c8cdb to b8c6c8b Compare July 14, 2025 10:51
Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

Looks good

@ruspl-afed
Copy link
Contributor Author

@laeubi TBH I do not understand what exactly is wrong with pipeline and how can I fix it

@laeubi
Copy link
Contributor

laeubi commented Jul 14, 2025

@laeubi laeubi merged commit cd13212 into eclipse-platform:master Jul 14, 2025
22 of 24 checks passed
@ruspl-afed ruspl-afed deleted the terminalconsoleviewmanager_optional branch July 14, 2025 15:00
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