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

Better documentation for test helpers, check config messages #1072

Merged
merged 7 commits into from
Apr 27, 2018

Conversation

justin808
Copy link
Member

@justin808 justin808 commented Apr 25, 2018

  • Throw if configuration.generated_assets_dir specified, and using
    webpacker, and if that doesn't match the public_output_path.
  • Otherwise, warn if generated_assets_dir is specified
  • Fix the setup for tests for spec/dummy so they automatically rebuild
    by correctly setting the source_path in the webpacker.yml
  • Updated documentation for the testing setup.

This change is Reviewable

@justin808 justin808 force-pushed the add-test-for-react-component-hash branch from 494e4c2 to 6c4ecde Compare April 25, 2018 08:23
* Throw if configuration.generated_assets_dir specified, and using
  webpacker, and if that doesn't match the public_output_path.
* Otherwise, warn if generated_assets_dir is specified
* Fix the setup for tests for spec/dummy so they automatically rebuild
  by correctly setting the source_path in the webpacker.yml
* Updated documentation for the testing setup.
@justin808 justin808 force-pushed the add-test-for-react-component-hash branch from 6c4ecde to 44a8a11 Compare April 25, 2018 08:33
@Judahmeek
Copy link
Contributor

Review status: 0 of 15 files reviewed at latest revision, all discussions resolved, some commit checks broke.


lib/react_on_rails/configuration.rb, line 40 at r1 (raw file):

      msg = <<-MSG.strip_heredoc
        Error configuring /config/initializers/react_on_rails.rb: You are using webpacker
        and you specified value for generated_assets_dir = #{@configuration.generated_assets_dir}

Should be ...and your specified value...


Comments from Reviewable

@justin808
Copy link
Member Author

Review status: 0 of 15 files reviewed at latest revision, all discussions resolved, some commit checks broke.


lib/react_on_rails/configuration.rb, line 40 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Should be ...and your specified value...

I'll fix!


Comments from Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at ?% when pulling 4d751ea on add-test-for-react-component-hash into b5b4960 on master.

@coveralls
Copy link

coveralls commented Apr 26, 2018

Coverage Status

Coverage remained the same at ?% when pulling d553ced on add-test-for-react-component-hash into b5b4960 on master.

@Judahmeek Judahmeek force-pushed the add-test-for-react-component-hash branch from 4d751ea to dc5657a Compare April 26, 2018 08:55
@mapreal19
Copy link
Member

:lgtm: some suggestions but feel free to merge and address this later if agreeing with suggestions


Reviewed 10 of 15 files at r1, 12 of 12 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


lib/react_on_rails/configuration.rb, line 25 at r3 (raw file):

  end

  def self.error_if_using_webpacker_and_generated_assets_dir_not_match_public_output_path

is there a test for this new method?


lib/react_on_rails/utils.rb, line 9 at r3 (raw file):

require "active_support/core_ext/string"

# Naming: goal is that path is a full_path and dir means relative to the rails root.

what about using full_path and relative_dir or anything better you can think of. The goal is to have self documenting code. If not it would be hard for a reader to find this comment since we could be using this naming pattern in other files


lib/react_on_rails/webpacker_utils.rb, line 7 at r3 (raw file):

    end

    # Full absolute path of webpacker output files.

instead of writing comments here what about having a spec with this info? In the spec we could see if returning a full path, pathname, etc. Easier to see future errors when upgrading webpacker too.


lib/react_on_rails/webpacker_utils.rb, line 14 at r3 (raw file):

    def self.bundle_js_file_path_from_webpacker(bundle_name)
      # Note Webpacker 3.4.3 manifest lookup is inside of the public_output_path

remove comment after upgrading 3.4.3?


Comments from Reviewable

@mapreal19 mapreal19 self-assigned this Apr 26, 2018
@justin808
Copy link
Member Author

added some tests


Review status: all files reviewed at latest revision, 4 unresolved discussions.


lib/react_on_rails/configuration.rb, line 25 at r3 (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

is there a test for this new method?

Yes, added one!


lib/react_on_rails/utils.rb, line 9 at r3 (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

what about using full_path and relative_dir or anything better you can think of. The goal is to have self documenting code. If not it would be hard for a reader to find this comment since we could be using this naming pattern in other files

good idea! but didn't change much, but one method name.


lib/react_on_rails/webpacker_utils.rb, line 7 at r3 (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

instead of writing comments here what about having a spec with this info? In the spec we could see if returning a full path, pathname, etc. Easier to see future errors when upgrading webpacker too.

Done.


lib/react_on_rails/webpacker_utils.rb, line 14 at r3 (raw file):

Previously, mapreal19 (Mario Pérez) wrote…

remove comment after upgrading 3.4.3?

Done.


Comments from Reviewable

@mapreal19
Copy link
Member

Reviewed 13 of 13 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


lib/react_on_rails/utils.rb, line 9 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

good idea! but didn't change much, but one method name.

👍


Comments from Reviewable

@mapreal19
Copy link
Member

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@justin808 justin808 merged commit 3a43054 into master Apr 27, 2018
@justin808 justin808 deleted the add-test-for-react-component-hash branch April 27, 2018 21:11
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.

4 participants