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

Use existing paths in Gruntfile.js #42

Closed
jayvdb opened this issue Nov 18, 2016 · 8 comments
Closed

Use existing paths in Gruntfile.js #42

jayvdb opened this issue Nov 18, 2016 · 8 comments
Assignees

Comments

@jayvdb
Copy link
Member

jayvdb commented Nov 18, 2016

Gruntfile.js contains pre-existing configuration which will exist in mature projects, which can be used to create the correct .coafile configuration. e.g. https://github.com/wikimedia/jquery.ime

		jshint: {
			options: {
				jshintrc: true
			},
			all: [
				'*.js',
				'src/*.js',
				'rules/**/*.js',
				'test/**/*.js'
			]
		},
		jscs: {
			fix: {
				options: {
					fix: true
				},
				src: '<%= jshint.all %>'
			},
			main: {
				src: '<%= jshint.all %>'
			}
		},
		csslint: {
			all: [
				'css/**/*.css'
			]
		},

Those top level keys (csslint, jscs, jshint) are constant, and directly map to names of linters; but the sub-keys change per grunt plugin, but all and main are quite common. IMO quickstart should still find all relevant files, and then emit a message to say which relevant files have been excluded due to Gruntfile.js.

There is also a common, but not standard, excerpt to correctly choose necessary linters:

	grunt.registerTask( 'lint', [ 'jshint', 'jscs:main', 'csslint' ] );

It would be very nice to warn if any of the lint sub-tasks can not be mapped to a bear in coala.

@satwikkansal
Copy link
Member

satwikkansal commented Jan 13, 2017

@jayvdb Just wanted to check if I understood it correctly. We've to do the following things

  • Look for a Gruntfile.js in the project's root and parse it if it exists.

  • Find the lint subtasks using regular expressions of the form grunt.registerTask( 'lint', [ 'jshint', 'jscs:main', 'csslint' ] ) or similar.

  • Try to map the discovered sub-task to a coala-bear, if failed, warn the users.

  • If there's a mapping to some coala bear, find that specific sub-task in the config Object's top level keys and get the paths corresponding to the all and main sub-keys.

  • Now, prompt the user to exclude the files found by quickstart not matching the paths extracted from the Gruntfile.js. These paths should be included in ignore field of the sections in generated .coafile corresponding to the mapped bear.

I discovered that in addition to all and main, at some places files sub-key is used. Example,

jshint: {
  // define the files to lint
  files: ['Gruntfile.js', 'src/**/*.js', 'test/**/*.js'],
  // configure JSHint (documented at http://www.jshint.com/docs/)
  options: {
    // more options here if you want to override JSHint defaults
    globals: {
      jQuery: true,
      console: true,
      module: true
    }
  }
}

Also I came across another kind of usage that involves src, sources sub-keys and default task.

module.exports = function(grunt) {
    grunt.initConfig({});

    grunt.loadNpmTasks('jshint');
    grunt.config('jshint', {
        sources: 'sources/**.js',
        tests: 'tests/**.js'
    });

    grunt.loadNpmTasks('less');
    grunt.config('less', {
        styles: {
            src: 'styles/index.less',
            dest: 'build/styles.css'
        }
    });

    grunt.registerTask('default', ['jshint', 'less']);
};

IMO we should look for the above patterns as well and also take care of the sub-keys like ignore and exclude in config object of Gruntfile.js.

@jayvdb
Copy link
Member Author

jayvdb commented Jan 13, 2017

That sounds very sensible. Re "Try to map the discovered sub-task to a coala-bear, if failed, warn the users." , IMO a "warning" isnt really required, and the focus should be to inform the users what parts of the Gruntfile.js were detected and enabled, and maybe a small note that -quickstart didnt recognise other parts of the Gruntfile.js , without too much detail or alarm.

@satwikkansal
Copy link
Member

@jayvdb Sorry to ping you again. I just had some doubts regarding:

  • Parsing the Gruntfile.js : I'm confused whether to use a regex to extract the concerned parts or to use a parser package like slimit and pyjsparser. IMO the later approach should be more reliable.
  • Filtering the bears according to the sub-tasks : I thought of defining a dictionary of linter (key) and list of bears that use those linters in constants.py.
    Another approach I thought of is to find the bears by matching the linte with the REQUIREMENTS set in the class declaration of the bear (checking if it's an instance of NPMRequirement and then matching the name).

@jayvdb
Copy link
Member Author

jayvdb commented Jan 22, 2017

@satwikkansal , either approach can work. either

  1. use a proper parser . See which existing parsers already parse these files. If there are bugs, raise them upstream.
  2. use regex to convert the .js into a correct JSON, and then use json module to load it.

re mapping tasks to bears, again both of your suggested approaches sound like they could work well.

Be bold. Try everything. Dont worry about performance. If it works, 👍 , and someone can rewrite it later if a better implementation comes along.

@jayvdb
Copy link
Member Author

jayvdb commented Jun 3, 2017

@satwikkansal , can you check whether the libraries you are using have:

  • CI for Windows
  • 'good' coverage (not necessarily 100%)

If not, it would be a good idea to try to do those things upstream, which hopefully doesnt result in any surprises, but ... if there are surprises you'll want to know about them now, not in phase 3 ;-)

#22 will need to be merged before your larger features are merged, otherwise the quality of your solution is unknown, and the bug reports will pour in during Google Code-In from Windows users if there are problems, catching us unprepared.

@satwikkansal
Copy link
Member

For JS related files I've been using pyjsparser but it doesn't have either of

  • CI for Windows
  • 100% coverage (for their tests.py, the coverage is 30%)

I'll create upstream issues regarding both of them.

@satwikkansal
Copy link
Member

satwikkansal commented Jun 3, 2017

#22 will need to be merged before your larger features are merged, otherwise the quality of your solution is unknown.

Alright, I had appveyor in later phases, but will now adapt my timeline accordingly :)

@satwikkansal
Copy link
Member

satwikkansal commented Nov 24, 2017

Completed with a more organized approach as a part of my GSoC project (relevant commits shown above), closing it now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants