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

Addon Storysource improvements #3040

Merged
merged 11 commits into from
Feb 20, 2018
Merged

Addon Storysource improvements #3040

merged 11 commits into from
Feb 20, 2018

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Feb 20, 2018

What I did

  • Use acorn to calculate locations instead of "line-column" pkg.
  • Remove ugly comments from the storysource (currently only eslint things)
  • Make "prettier" and "uglyComments" configurable in loader

}

function prettifyCode(source) {
return prettier.format(source, {
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it optional and configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the prettier or the whole comments removal? Also. why?

Copy link
Member

Choose a reason for hiding this comment

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

The prettier. Users may have their own preferences about how their code should be formatted

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I thought we won't expose it to users 😂 But I can make it configurable.

Copy link
Member

Choose a reason for hiding this comment

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

I thought we won't expose it to users

Why? I would like to use it in my project (and we don't use semicolons =)

Copy link
Member Author

@igor-dv igor-dv Feb 20, 2018

Choose a reason for hiding this comment

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

👌

Probably I needed to spend a bit more time on writing a Readme 🤔

let lastIndex = 0;
const parts = [source];

const isUgly = comment => comment.value.trim().indexOf('eslint-') === 0;
Copy link
Member

Choose a reason for hiding this comment

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

also "global", "flow", "$FlowFixMe", probably others. Let's make it an array of regexp patterns, probably also configurable from outside

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added "global" and "eslint" as defaults. Do you think we need to support flow as well? I'm not familiar with its comments syntax.

Copy link
Member

Choose a reason for hiding this comment

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

No, it's good defaults

@codecov
Copy link

codecov bot commented Feb 20, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@3edc7d1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #3040   +/-   ##
========================================
  Coverage          ?   92.5%           
========================================
  Files             ?       6           
  Lines             ?      40           
  Branches          ?       2           
========================================
  Hits              ?      37           
  Misses            ?       2           
  Partials          ?       1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3edc7d1...8575ebf. Read the comment docs.

};

function isUglyComment(comment, uglyCommentsRegex) {
return uglyCommentsRegex.find(regex => regex.test(comment));
Copy link
Member

Choose a reason for hiding this comment

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

.some not .find

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

Let's fix array method and update README — and we're good to go

@Hypnosphi
Copy link
Member

#!/bin/bash -eo pipefail
yarn lint
yarn run v1.3.2
$ yarn lint:js . && yarn lint:ts **/*.ts && yarn lint:md .
$ cross-env NODE_ENV=production eslint --cache --cache-location=.cache/eslint --ext .js,.jsx,.json --report-unused-disable-directives .
$ tslint -p . -c tslint.json -t stylish '**/*.ts'
$ remark -q .
addons/storysource/README.md
  1:1  warning  Missing newline character at end of file  final-newline  remark-lint

⚠ 1 warning
Done in 41.52s.

Please fix remark warning

@Hypnosphi Hypnosphi merged commit 3fb98f5 into master Feb 20, 2018
@Hypnosphi Hypnosphi deleted the storysource-adjustments branch February 20, 2018 17:49
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.

2 participants