janosrusiczki / japr

Jekyll Asset Pipeline Reborn - Powerful asset pipeline for Jekyll that collects, converts and compresses JavaScript and CSS assets.
MIT License
51 stars 9 forks source link

Rubocop fixes #23

Closed janosrusiczki closed 6 years ago

janosrusiczki commented 6 years ago

Fix Rubocop errors / offenses.

janosrusiczki commented 6 years ago

@szemek FYI I started working on this.

szemek commented 6 years ago

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

janosrusiczki commented 6 years ago

@szemek and now for the hard part :smile:

szemek commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

Coverage Status

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

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling 662174783865b3399dbd7aa64edaf9d805d15cb4 on rubocop-fixes into on master.

janosrusiczki commented 6 years ago

@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. :smile:

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

coveralls commented 6 years ago

Coverage Status

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

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling c79595665fdcf74b539ad2329baf61672c9b5b4f on rubocop-fixes into on master.

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling ba207203eab12daa6c2357bf229ec226b38e5388 on rubocop-fixes into on master.

janosrusiczki commented 6 years ago

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

coveralls commented 6 years ago

Coverage Status

Changes Unknown when pulling b5f6f34ba686eaa0fc5b0a2e0d31bfbc19e72493 on rubocop-fixes into on master.