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

Copy in the elvis files #2

Merged
merged 3 commits into from
Nov 25, 2015
Merged

Copy in the elvis files #2

merged 3 commits into from
Nov 25, 2015

Conversation

Licenser
Copy link
Contributor

This commit copies all 'core' elvis file form the elvis repository
including tests. It clens up code that was related to non core
tasks and removes the tests for that code.

This commit copies all 'core' elvis file form the elvis repository
including tests. It clens up code that was related to non core
tasks and removes the tests for that code.
@jfacorro
Copy link
Contributor

@Licenser Hi, thanks for your work on this PR! I have a few ideas and comments I wanted to share.

I was thinking that maybe we could make elvis_core leaner, although I don't have a clear idea of exactly what that looks like. For example all output to screen is actually related to the command-line tool, so it doesn't feel right that there are calls to elvis_utils:info/1 and friends in elvis_core.

My point is that elvis_core should only provide core functionality. Clearly defining that core functionality is the challenge I guess.

There are some modules that:

  1. could be brought from elvis without modification,
  2. others that might need minor changes and
  3. a few could use a rewrite to expose a better agnostic API.

I will list the modules I think belong to each group so we can start a discussion:

1. Without modification

  • elvis_code
  • elvis_config
  • elvis_file
  • elvis_style

2. Minor changes

  • elvis_utils: we could remove all of the function that handle the output since that is not really a core feature.
  • elvis_result: the print/1 function could be implemented by the user of elvis_core. We may have to modify the way we expose the types and their structure in this module.
  • elvis_project (only because it has a couple of calls to elvis_utils:error_prn/2)

3. Rewrite

  • elvis (which I think should be renamed to elvis_core): maybe the module doesn't need a full rewrite, but it would be a good idea to remove the functions where no Config is provided so that elvis_core clients explicitly use elvis_config:default/0. It would also be a good idea to have an additional Options argument in all API functions. A possible option would be a callback function used to report partial results or logging messages, which could be disabled by not providing a value for such options.

We can merge this PR and work from there. Or we could discuss what would be the roadmap for elvis_core and create issues to organize work. Personally I prefer the second option.

One last detail, the file test/examples/pull_request.js is used for testing the webhook functionality, so it shouldn't be included in this project.

@brujo what do you think?

@elbrujohalcon
Copy link
Member

@jfacorro I think we can merge this PR, add your comments in an issue (or more than one) and start from here.

@elbrujohalcon
Copy link
Member

@jfacorro and I think elvis module should be renamed elvis_core, and an elvis module should be created in the elvis repo, which will use elvis_core for the core functionality and expose the extra stuff as it is now.

@jfacorro
Copy link
Contributor

and I think elvis module should be renamed elvis_core, and an elvis module should be created in the elvis repo, which will use elvis_core for the core functionality and expose the extra stuff as it is now.

@elbrujohalcon Yeah, I included that in the comments.

@jfacorro
Copy link
Contributor

@Licenser To sum up. Please remove the file test/examples/pull_request.js and rename the elvis module to elvis_core. Thanks!

Remove unused test example and some unused test code,
renamed elvis.erl to elvis_core.erl with all remifications of that
@jfacorro jfacorro assigned jfacorro and unassigned jfacorro Nov 24, 2015
@Licenser
Copy link
Contributor Author

hmm the elvis gadget check isn't finishing o.O

@jfacorro
Copy link
Contributor

@Licenser There seems to be a problem with gadget when using elvis in this project. I'll check the logs to find out what is the problem and let you know.

@jfacorro jfacorro assigned jfacorro and unassigned jfacorro Nov 25, 2015
jfacorro added a commit that referenced this pull request Nov 25, 2015
@jfacorro jfacorro merged commit c5e813e into inaka:master Nov 25, 2015
@jfacorro
Copy link
Contributor

@Licenser Merged!

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.

3 participants