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

fix #844 to remove all macros #860

Merged
merged 5 commits into from
Nov 1, 2021
Merged

fix #844 to remove all macros #860

merged 5 commits into from
Nov 1, 2021

Conversation

sandraros
Copy link
Collaborator

fix #844 to remove all macros

fix #844 to remove all macros
Copy link
Member

@AndreaBorgia-Abo AndreaBorgia-Abo left a comment

Choose a reason for hiding this comment

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

Other than a few minor issues with mixed-case, this is good.
The "color" flag issue was already present: as long as we preserve the (possibly broken) logic it shouldln't matter.

src/demos/zdemo_excel27.prog.abap Outdated Show resolved Hide resolved
src/demos/zdemo_excel27.prog.abap Outdated Show resolved Hide resolved
src/demos/zdemo_excel27.prog.abap Outdated Show resolved Hide resolved
src/not_cloud/zcl_excel_ole.clas.abap Outdated Show resolved Hide resolved
src/not_cloud/zcl_excel_ole.clas.abap Outdated Show resolved Hide resolved
src/zcl_excel_worksheet.clas.abap Outdated Show resolved Hide resolved
src/zcl_excel_worksheet.clas.abap Outdated Show resolved Hide resolved
src/zcl_excel_worksheet.clas.abap Outdated Show resolved Hide resolved
src/zcl_excel_worksheet.clas.abap Outdated Show resolved Hide resolved
src/zcl_excel_worksheet.clas.abap Outdated Show resolved Hide resolved
@larshp
Copy link
Member

larshp commented Oct 31, 2021

in abaplint.json, suggest changing

    "avoid_use": {
      "define": false,

to

    "avoid_use": {
      "define": true,

that way all future pull request will be rejected if they contain macros/define

@sandraros sandraros closed this Oct 31, 2021
@sandraros sandraros deleted the sandraros/macros2 branch October 31, 2021 16:20
@sandraros sandraros restored the sandraros/macros2 branch October 31, 2021 16:22
@sandraros sandraros reopened this Oct 31, 2021
@sandraros
Copy link
Collaborator Author

sandraros commented Oct 31, 2021

I did the changes you suggested. Could you review again? Thanks!
NB: I probably should not have clicked "resolve conversation" for all your suggestions, and left it up to you. It's just that I'm discovering the process so not sure of what I'm doing. Note that you can also "suggest specific changes to lines of code, which the author can apply directly", maybe that will help make the review and corrections smoother.

@sandraros
Copy link
Collaborator Author

sandraros commented Oct 31, 2021

Two more things that I just saw and fixed:

  1. A bug I introduced, that I could reproduce with this code in case 1 (borders were shown):
    REPORT.
    DATA: gc_save_file_name TYPE string VALUE 'test.xlsx'.
    INCLUDE zdemo_excel_outputopt_incl.
    START-OF-SELECTION.
      TRY.
          DATA(lo_excel) = NEW zcl_excel( ).
          DATA(lo_worksheet) = lo_excel->get_active_worksheet( ).
          lo_worksheet->set_cell( ip_column = 'B' ip_row = 2 ip_value = CONV decfloat16( 1 ) ).
          " 1) cell should have no border
          lo_worksheet->change_cell_style(
                ip_column             = 'B'
                ip_row                = 2
                ip_borders_allborders = VALUE #( border_style = zcl_excel_style_border=>c_border_thick
                                                 border_color = VALUE #( rgb = zcl_excel_style_color=>c_darkred ) )
                ip_xborders_allborders = VALUE #( border_style = abap_false
                                                  border_color = VALUE #( rgb = abap_false ) )
                ip_xborders = VALUE #( allborders = VALUE #( border_style = abap_true
                                                             border_color = VALUE #( rgb = abap_true ) ) ) ).
          " 2) cell should have borders
          lo_worksheet->change_cell_style(
                ip_column             = 'D'
                ip_row                = 2
                ip_borders_allborders = VALUE #( border_style = zcl_excel_style_border=>c_border_thick
                                                 border_color = VALUE #( rgb = zcl_excel_style_color=>c_darkred ) )
                ip_xborders_allborders = VALUE #( border_style = abap_true
                                                  border_color = VALUE #( rgb = abap_true ) ) ).
          " 3) cell should have borders
          lo_worksheet->change_cell_style(
                ip_column             = 'F'
                ip_row                = 2
                ip_borders_allborders = VALUE #( border_style = zcl_excel_style_border=>c_border_thick
                                                 border_color = VALUE #( rgb = zcl_excel_style_color=>c_darkred ) ) ).
          lcl_output=>output( cl_excel = lo_excel ).
        CATCH cx_root INTO DATA(lx_root).
          MESSAGE lx_root TYPE 'I' DISPLAY LIKE 'E'.
      ENDTRY.
  2. I also removed the changing parameter cs_borderx from method move_supplied_borders, which was useless.

@AndreaBorgia-Abo
Copy link
Member

Note that you can also "suggest specific changes to lines of code, which the author can apply directly", maybe that will help make the review and corrections smoother.

Ha, I didn't know that, "insert a suggestion" seems useful, I'll try to use it, thanks.

Let me check the new changes...

Copy link
Member

@AndreaBorgia-Abo AndreaBorgia-Abo left a comment

Choose a reason for hiding this comment

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

LGTM

@AndreaBorgia-Abo AndreaBorgia-Abo merged commit 3b2423e into master Nov 1, 2021
@AndreaBorgia-Abo AndreaBorgia-Abo deleted the sandraros/macros2 branch November 1, 2021 15:57
@AndreaBorgia-Abo
Copy link
Member

@larshp I wanted to check which bug @sandraros introduced and then fixed but I couldn't find a way to check out a specific commit, only switching branches and tags are supported if I'm not mistaken. I did the next best thing, confirming the problem is indeed gone.

@larshp
Copy link
Member

larshp commented Nov 1, 2021

this part should help checking out a specific commit,
image

@AndreaBorgia-Abo
Copy link
Member

Gosh, I kept looking in the menus... :D
Thanks.

@AndreaBorgia-Abo
Copy link
Member

AndreaBorgia-Abo commented Nov 4, 2021

this part should help checking out a specific commit,

Close but no cigar: "Cannot find initial commit. Too many commits. Action not possible. "
Selection by PR works, though I'm not sure if it is any different than switching to the PR branch.

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.

Clean code: should we replace macros with methods, and why?
3 participants