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

Move report formatter and charting to core #19873

Merged

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Feb 26, 2020

Note, for happy reviewing, please review the main part of the PR in commit "Namespace all the things." without whitespace changes.

This and the UI classic PR resolves: #19674 (missing ReportFormatter for backend workers)

Also, related to #19863 (move code from UI-classic that can be called from either the frontend or backend

Thanks @bdunne for some guidance on naming things and deciding if this be should a plugin or not (tldr; too many current references to classes embedded in this code so we can't plugin-ize yet)

Also, this might help visualize the layout.

Before, in ui-classic:

lib
├── charting
│   └── c3_charting.rb
├── charting.rb
...
├── report_formatter
│   ├── c3.rb
│   ├── c3_series.rb
│   ├── chart_common.rb
│   ├── converter.rb
│   ├── html.rb
│   ├── report_renderer.rb
│   ├── text.rb
│   ├── timeline.rb
│   └── timeline_message.rb
├── report_formatter.rb
...

After, in core:

lib
...
├── charting.rb # Deprecate old Charting
├── manageiq
    ...
│   ├── reporting
│   │   ├── charting
│   │   │   └── c3_charting.rb
│   │   ├── charting.rb
│   │   ├── formatter
│   │   │   ├── c3.rb
│   │   │   ├── c3_helper.rb
│   │   │   ├── c3_series.rb
│   │   │   ├── chart_common.rb
│   │   │   ├── converter.rb
│   │   │   ├── html.rb
│   │   │   ├── report_renderer.rb
│   │   │   ├── text.rb
│   │   │   ├── timeline.rb
│   │   │   └── timeline_message.rb
│   │   └── formatter.rb
    ...
...
├── report_formatter.rb. # Deprecate old ReportFormatter

@jrafanie
Copy link
Member Author

cc @PanSpagetka

# * We assign the old toplevel constant to the new constant.
# * We can't include rails deprecate_constant globally, so we use ruby's.
Charting = ManageIQ::Reporting::Charting
Object.deprecate_constant :Charting
Copy link
Member Author

@jrafanie jrafanie Feb 26, 2020

Choose a reason for hiding this comment

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

Reviewers, let me explain this. Rails has deprecate_constant that you can use by including ActiveSupport::Deprecation::DeprecatedConstantAccessor in your class/module. This is great and works well in the old ReportFormatter, see the lib/manageiq/reporting/formatter.rb. The major benefit is it tells you what is deprecated and what's the new behavior to use.

Unfortunately, you can't include that module globally, and we have two global constants, Charting and ReportFormatter to deprecate. Ruby has deprecate_constant, which we can call on Object to ensure any references to Charting ReportFormatter at the top level do raise deprecations. The ruby version doesn't need to include any modules but it also just tells you it's deprecated without any suggestions as to the solution. I think this is the best we can do. Maybe someone knows something better?

So, here, we setup the old Constant, set to the new one and deprecate the old one.

@@ -57,4 +57,5 @@
/content @bdunne
/config @jrafanie
/db @agrare @carboni @bdunne
/lib/manageiq/reporting @panspagetka @jrafanie
Copy link
Member Author

Choose a reason for hiding this comment

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

Add @PanSpagetka and I as the reviewers for this directory from a code and code loading side.

# * We assign the old toplevel constant to the new constant.
# * We can't include rails deprecate_constant globally, so we use ruby's.
ReportFormatter = ManageIQ::Reporting::Formatter
Object.deprecate_constant :ReportFormatter
Copy link
Member Author

Choose a reason for hiding this comment

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

See comment on lib/charting.rb... same thing is happening here.

@bdunne
Copy link
Member

bdunne commented Feb 26, 2020

Can you add the typical extraction message to the first commit? Something like:

Extract class MiqReportFormats(transferred from ManageIQ/manageiq@9415b33cf35c72e8cff43c4719f9927500a3f5a2)

@jrafanie
Copy link
Member Author

I updated the description with a before/after file layout tree to help visualize the changes.

module Formatter
class Converter
# generate a ruport table from an array of hashes where the keys are the column names
def self.hashes2table(hashes, options)
Copy link
Member

Choose a reason for hiding this comment

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

😆 Did you show this 2 @Fryguy ?

Copy link
Member Author

Choose a reason for hiding this comment

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

beautifully_named_methods4the_win

Related to:
ManageIQ#19674 (missing ReportFormatter for
backend workers)
ManageIQ#19863 (move code from UI-classic
that can be called from either the frontend or backend)

Transferred from ManageIQ/manageiq-ui-classic@24fc2be
Related to:
ManageIQ#19674 (missing ReportFormatter for
backend workers)
ManageIQ#19863 (move code from UI-classic
that can be called from either the frontend or backend)
@jrafanie jrafanie force-pushed the move_report_formatter_charting_to_core branch from 60e76f9 to a676763 Compare February 26, 2020 19:28
@bdunne
Copy link
Member

bdunne commented Feb 26, 2020

image
👋 Bye formater

From ManageIQ#19863
Related to:
ManageIQ#19674 (missing ReportFormatter for
backend workers)
ManageIQ#19863 (move code from UI-classic
that can be called from either the frontend or backend)
From ManageIQ#19863
Related to:
ManageIQ#19674 (missing ReportFormatter for
backend workers)
ManageIQ#19863 (move code from UI-classic
that can be called from either the frontend or backend)
@jrafanie jrafanie force-pushed the move_report_formatter_charting_to_core branch from a676763 to bf0bfd4 Compare February 26, 2020 22:10
@miq-bot
Copy link
Member

miq-bot commented Feb 26, 2020

Some comments on commits jrafanie/manageiq@2a4d36a~...bf0bfd4

spec/lib/manageiq/reporting/formatter/c3_spec.rb

  • ⚠️ - 21 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 43 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 59 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 69 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 79 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 95 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

spec/lib/manageiq/reporting/formatter/chart_common_spec.rb

  • ⚠️ - 4 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 50 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Feb 26, 2020

Checked commits jrafanie/manageiq@2a4d36a~...bf0bfd4 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.20.0, and yamllint
26 files checked, 206 offenses detected

lib/charting.rb

lib/manageiq/reporting/charting.rb

  • ❗ - Line 6, Col 18 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

lib/manageiq/reporting/charting/c3_charting.rb

lib/manageiq/reporting/formatter.rb

lib/manageiq/reporting/formatter/c3.rb

lib/manageiq/reporting/formatter/c3_helper.rb

lib/manageiq/reporting/formatter/c3_series.rb

lib/manageiq/reporting/formatter/chart_common.rb

lib/manageiq/reporting/formatter/html.rb

lib/manageiq/reporting/formatter/text.rb

lib/manageiq/reporting/formatter/timeline.rb

  • ❗ - Line 105, Col 27 - Layout/SpaceAroundOperators - Operator = should be surrounded by a single space.
  • ❗ - Line 105, Col 28 - Layout/ExtraSpacing - Unnecessary spacing detected.
  • ❗ - Line 108, Col 15 - Style/NegatedIf - Favor unless over if for negative conditions.
  • ❗ - Line 126, Col 36 - Style/UnneededCondition - Use double pipes || instead.
  • ❗ - Line 172, Col 13 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
  • ❗ - Line 23, Col 48 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.xmlschema, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.httpdate, Time.now.to_i, Time.now.to_f instead.
  • ❗ - Line 32, Col 11 - Style/ConditionalAssignment - Use the return of the conditional for variable assignment and comparison.
  • ❗ - Line 32, Col 41 - Layout/ExtraSpacing - Unnecessary spacing detected.
  • ❗ - Line 33, Col 69 - Layout/ExtraSpacing - Unnecessary spacing detected.
  • ❗ - Line 35, Col 31 - Layout/ExtraSpacing - Unnecessary spacing detected.
  • ❗ - Line 61, Col 33 - Layout/ExtraSpacing - Unnecessary spacing detected.
  • ❗ - Line 66, Col 61 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.xmlschema, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.httpdate, Time.now.to_i, Time.now.to_f instead.
  • ❗ - Line 75, Col 31 - Layout/ExtraSpacing - Unnecessary spacing detected.
  • ❗ - Line 76, Col 11 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
  • ❗ - Line 78, Col 62 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 80, Col 107 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 80, Col 62 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 83, Col 123 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 83, Col 38 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 83, Col 82 - Rails/TimeZone - Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.xmlschema, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.httpdate, Time.now.to_i, Time.now.to_f instead.
  • ❗ - Line 84, Col 64 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 86, Col 109 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 86, Col 64 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 86, Col 84 - Style/IfInsideElse - Convert if nested inside else to elsif.
  • ❗ - Line 89, Col 107 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 89, Col 62 - Rails/Date - Do not use to_time on Date objects, because they know nothing about the time zone in use.
  • ❗ - Line 89, Col 82 - Style/IfInsideElse - Convert if nested inside else to elsif.
  • ❗ - Line 92, Col 23 - Layout/ExtraSpacing - Unnecessary spacing detected.
  • ❗ - Line 93, Col 19 - Rails/DynamicFindBy - Use find_by instead of dynamic find_by_id.

lib/manageiq/reporting/formatter/timeline_message.rb

lib/report_formatter.rb

spec/lib/manageiq/reporting/formatter/c3_spec.rb

spec/lib/manageiq/reporting/formatter/chart_common_spec.rb

spec/lib/manageiq/reporting/formatter/text_spec.rb

spec/lib/manageiq/reporting/formatter/timeline_spec.rb

spec/models/miq_widget/chart_content_spec.rb

spec/support/report_helper.rb

@miq-bot
Copy link
Member

miq-bot commented Feb 26, 2020

...continued

@chessbyte chessbyte self-assigned this Feb 27, 2020
@chessbyte chessbyte merged commit c771f4e into ManageIQ:master Feb 27, 2020
@chessbyte chessbyte added this to the Sprint 131 Ending Mar 2, 2020 milestone Feb 27, 2020
@jrafanie jrafanie deleted the move_report_formatter_charting_to_core branch February 27, 2020 15:17
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.

[Bug] uninitialized constant MiqReport::Formatters::Graph::ReportFormatter
4 participants