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

Render console exceptions using standard HTML exception renderer #1246

Merged
merged 5 commits into from
Jun 9, 2020
Merged

Render console exceptions using standard HTML exception renderer #1246

merged 5 commits into from
Jun 9, 2020

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Jun 5, 2020

Reuse HTML table rendering for outputting console stack traces, technically improving code reuse.

Current behaviour:

Screenshot 2020-06-08 at 15 46 09

Example after change:

image

Other info

no BC break from code point of view (only different UX)

App::renderExceptionHTMLText() was introduced not that long by my in another refactoring...

@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #1246 into develop will increase coverage by 0.00%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1246   +/-   ##
==========================================
  Coverage      72.83%   72.84%           
+ Complexity      2557     2554    -3     
==========================================
  Files            130      130           
  Lines           6270     6267    -3     
==========================================
- Hits            4567     4565    -2     
+ Misses          1703     1702    -1     
Impacted Files Coverage Δ Complexity Δ
src/App.php 82.06% <ø> (-0.10%) 146.00 <0.00> (-1.00)
src/Console.php 81.61% <92.30%> (-0.87%) 44.00 <2.00> (-2.00)
src/jsExpression.php 87.34% <0.00%> (+2.53%) 31.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a59506b...1730fce. Read the comment docs.

@mvorisek mvorisek added the RTM label Jun 5, 2020
@mvorisek mvorisek marked this pull request as ready for review June 5, 2020 23:39
src/Console.php Outdated
foreach ($lines as $line) {
$this->outputHTML($line);
}
$this->outputHTML('<div style="white-space: normal; font-family: Lato,\'Helvetica Neue\',Arial,Helvetica,sans-serif;">{0}</div>', [$this->app->renderExceptionHTML($e)]);
Copy link
Member Author

Choose a reason for hiding this comment

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

@ibelar ok with you? do you know a better way to reset the css styles/font?

@romaninsh
Copy link
Member

romaninsh commented Jun 8, 2020

I have added "current" screenshot to the description. Looking at them side-by-side, I still kinda like the first one for the console output. Current output can be much easier copy pasted, e.g.:

Executing test process...
Now trying something dangerous..
direct output is captured
--[ Critical Error ]---------------------------
atk4\core\Exception: BOOM - exceptions are caught 

Stack Trace:
           demos/interactive/console.php:  41 
                       demos/interactive:     atk4\ui\demo\{closure}(
                                        atk4\ui\Console (atk_admin_tabs_tabssubview_console))
                         src/Console.php: 109 call_user_func()
                       demos/interactive:     - atk_admin_tabs_tabssubview_console atk4\ui\Console::atk4\ui\{closure}()
                      src/jsCallback.php: 104 call_user_func_array()
                       demos/interactive:     - atk_admin_tabs_tabssubview_console_jssse atk4\ui\jsCallback::atk4\ui\{closure}()
                        src/Callback.php: 126 call_user_func_array()
                      src/jsCallback.php: 116 - atk_admin_tabs_tabssubview_console_jssse atk4\ui\Callback::set()
                         src/Console.php: 123 - atk_admin_tabs_tabssubview_console_jssse atk4\ui\jsCallback::set()
           demos/interactive/console.php:  42 - atk_admin_tabs_tabssubview_console atk4\ui\Console::set()

@mvorisek
Copy link
Member Author

mvorisek commented Jun 8, 2020

@romaninsh argument is nonsense, as you can say exactly the same for normal HTML exception

also the current HTML is quite copy-pasteable, so your argument is not only nonsense, but also demolished now:

Executing test process...
Now trying something dangerous..
direct output is captured

Critical Error
atk4\core\Exception : BOOM - exceptions are caught
Stack Trace
#	File	Object	Method
	demos/interactive/console.php:42 		
9 			atk4\ui\demo\{closure}()
8 	src/Console.php:109 		call_user_func()
7 		atk_admin_tabs_tabssubview_console 	atk4\ui\{closure}()
6 	src/jsCallback.php:104 		call_user_func_array()
5 		atk_admin_tabs_tabssubview_console_jssse 	atk4\ui\{closure}()
4 	src/Callback.php:126 		call_user_func_array()
3 	src/jsCallback.php:116 	atk_admin_tabs_tabssubview_console_jssse 	set()
2 	src/Console.php:120 	atk_admin_tabs_tabssubview_console_jssse 	set()
1 	demos/interactive/console.php:43 	atk_admin_tabs_tabssubview_console 	set()

anyway, we are in screenshots time :D :D

@romaninsh
Copy link
Member

@mvorisek I'm entitled to my own opinion (prefer text-based output), just like you are entitled to yours.

Also - I do not understand the benefit of this change.

@mvorisek
Copy link
Member Author

mvorisek commented Jun 8, 2020

I do not understand the benefit of this change.

@romaninsh currently, the renderers are splitted between several classes that can not be further extended, so they need currently a lot of code like:
https://github.com/atk4/ui/pull/1246/files#diff-58980ac23a3a9a0b92abd9805eed1e78L1032

and this was the primary motivation to look into this

I have edited this several times already, it is really hard to maintain and I think the overall result is also better, we should target one implementation for everything, not n partially working ones

Copy link
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

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

LGTM

@DarkSide666 DarkSide666 merged commit cbcb660 into atk4:develop Jun 9, 2020
@mvorisek mvorisek deleted the remove_HTMLText branch June 9, 2020 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants