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

Refactoring and added codeclimate #21

Merged
merged 5 commits into from
Oct 22, 2016
Merged

Refactoring and added codeclimate #21

merged 5 commits into from
Oct 22, 2016

Conversation

Schultzer
Copy link
Collaborator

@Schultzer Schultzer commented Oct 13, 2016

Okay. so we need to figure out the rules we should use in our .eslintrc, .codeclimate.yml maybe CSSlint?
so we can figure out what we need to refactor and what is not necessary I need @danschultzer your 👀 on that task.

  • Ruleset for .eslintrc and .codeclimate.yml
  • Removed .eslintrc and codeclimate.yml
  • Refactoring
  • Indention
  • Single quotetation marks 'string' for strings in our code base
  • Move app menu from background.js to it's own app_menu_template.js

@codecov-io
Copy link

codecov-io commented Oct 17, 2016

Current coverage is 93.37% (diff: 100%)

Merging #21 into master will not change coverage

@@             master        #21   diff @@
==========================================
  Files             3          3          
  Lines           166        166          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            155        155          
  Misses           11         11          
  Partials          0          0          

Powered by Codecov. Last update 526d348...8ba83da

@Schultzer Schultzer force-pushed the Refactor-Codeclimate branch 2 times, most recently from dc659a9 to 54df077 Compare October 21, 2016 09:20
@danschultzer
Copy link
Owner

Remove code climate and eslint files, and remove codecliamte from readme.

@@ -143,21 +143,39 @@ if (env.name !== 'production') {
app.setPath('userData', userDataPath + ' (' + env.name + ')');
}

function openWindow(windowName, parentWindow) {
Copy link
Owner

@danschultzer danschultzer Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just make this opts in the first place instead of using parentWindow?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, all this should be refactored more.

});

it("handles \\t in values", function() {
it('handles \\t in values', function() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\\t should be changed to \t?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. We want to escape the tab in the string for readability.

@danschultzer
Copy link
Owner

Refactored openWindow method. The rest looks fine.

@danschultzer danschultzer mentioned this pull request Oct 21, 2016
9 tasks
@danschultzer
Copy link
Owner

This is a massive change PR, so we need to merge this in fast. However, we should take some time to see if there's anything that needs to be refactored or cleaned up.

@Schultzer Schultzer force-pushed the Refactor-Codeclimate branch 2 times, most recently from 0cb688e to 77c25f3 Compare October 21, 2016 20:51
Schultzer and others added 5 commits October 21, 2016 20:18
Removed .eslintrc and .codeclimate.yml
Removed Error: string in title tag it’s shows up twice in tooltip.
@Schultzer Schultzer merged commit 6393e1b into master Oct 22, 2016
@Schultzer Schultzer deleted the Refactor-Codeclimate branch October 22, 2016 03:45
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.

3 participants