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

Rubocop fixes #23

Merged
merged 17 commits into from
Dec 3, 2017
Merged

Rubocop fixes #23

merged 17 commits into from
Dec 3, 2017

Conversation

janosrusiczki
Copy link
Owner

Fix Rubocop errors / offenses.

@janosrusiczki
Copy link
Owner Author

@szemek FYI I started working on this.

@janosrusiczki janosrusiczki self-assigned this Nov 28, 2017
@szemek
Copy link
Collaborator

szemek commented Nov 28, 2017

@janosrusiczki I added few commits and reduced amount of offences to 4.

@janosrusiczki
Copy link
Owner Author

@szemek and now for the hard part 😄

@szemek
Copy link
Collaborator

szemek commented Nov 28, 2017

@janosrusiczki what do you think about following refactoring for render method in lib/japr/extensions/liquid/liquid_block_extensions.rb ?

    def render(context)
      site = context.registers[:site]
      config = site.config.fetch('asset_pipeline', {})

      # Run Jekyll Asset Pipeline
      pipeline, cached = run_pipeline(site, config)

      return nil unless pipeline.is_a?(Pipeline)

      # Prevent Jekyll from cleaning up saved assets if new pipeline
      preserve_assets(site, config, pipeline) unless cached

      # Return HTML tag pointing to asset
      pipeline.html
    end

    private

    def run_pipeline(site, config)
      Pipeline.run(nodelist.first, @markup.strip, site.source, site.dest,
                   self.class.tag_name, self.class.output_type, config)
    end

    def preserve_assets(site, config, pipeline)
      pipeline.assets.each do |asset|
        config = JAPR::DEFAULTS.merge(config)
        staging_path = File.expand_path(File.join(site.source,
                                                  config['staging_path']))
        site.static_files << Jekyll::StaticFile.new(site, staging_path,
                                                    asset.output_path,
                                                    asset.filename)
      end
    end

@janosrusiczki
Copy link
Owner Author

@szemek It looks good. Let's go ahead and make the change. I got stuck yesterday evening by updating the rake dependency to 12.0 which gives method redefined warnings around this mock class: https://github.com/janosrusiczki/japr/blob/master/spec/extensions/liquid/liquid_block_extensions_spec.rb#L24 I think I'll revert for the time being and concentrate on the Rubocop stuff.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 17aaf1e on rubocop-fixes into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6621747 on rubocop-fixes into ** on master**.

@janosrusiczki
Copy link
Owner Author

janosrusiczki commented Dec 3, 2017

@szemek I'm back after a little sick time. 🤧 Fixed one ABC complexity issue, but now the class is too long. I'm not sure I can fix the last two issues (besides the class length). I will try to fix the Codeclimate issues in the meantime. I have the ambition to have these passing before I build a new gem version. 😄

Edit: Removed the ClassLength cop, deactivated complexity check on Codeclimate. Two issues remain to fix.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9999d6b on rubocop-fixes into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c795956 on rubocop-fixes into ** on master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling ba20720 on rubocop-fixes into ** on master**.

@janosrusiczki
Copy link
Owner Author

@szemek It looks like I made it, please have a look and give me the green if it's mergeable.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b5f6f34 on rubocop-fixes into ** on master**.

Copy link
Collaborator

@szemek szemek 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 to me 👍

@janosrusiczki janosrusiczki merged commit 9762fe4 into master Dec 3, 2017
@janosrusiczki janosrusiczki deleted the rubocop-fixes branch December 3, 2017 16: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.

None yet

3 participants