Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

Add generator-superset #21

Merged
merged 6 commits into from
Nov 8, 2018
Merged

Add generator-superset #21

merged 6 commits into from
Nov 8, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Nov 5, 2018

🏆 Enhancements

  • Add generator-superset package for scaffolding.
  • This package will be published as @superset-ui/generator-superset.
  • Update build script to skip babel tasks for generator-superset, as it is a node application.
  • Use jest project configuration to separate generator-superset from others and run in testEnvironment: "node". (Although that was not the real issue that demolished CI in the early attempts.)

Note about yeoman testing and interference with other packages

The key thing to add to yeoman tests, which are not normally included even from official yeoman generator-generator, is to change working directory back to original working directory after the test has completed. yeoman tests switch to tmp directory and write files there. Usually this is fine for solo package. However, for a monorepo like this one, it made jest confuses with current directory (being in tmp directory instead of superset-ui root) and interferes with other tests in sibling packages that are run after the yeoman tests.

By adding the directory switch in afterAll() block. Everything works fine again! 🍾

image

Many yaks were shaved in the making of this pull request.

@codecov
Copy link

codecov bot commented Nov 5, 2018

Codecov Report

Merging #21 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #21   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          37     39    +2     
  Lines         325    339   +14     
=====================================
+ Hits          325    339   +14
Impacted Files Coverage Δ
...ges/generator-superset/generators/package/index.js 100% <100%> (ø)
...ackages/generator-superset/generators/app/index.js 100% <100%> (ø)

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 3196ab1...c66fdf1. Read the comment docs.

@kristw kristw added the WIP Work in progress label Nov 5, 2018
@kristw kristw self-assigned this Nov 5, 2018
@kristw kristw force-pushed the kristw-generator branch 4 times, most recently from 1c99e38 to 6331bac Compare November 7, 2018 06:10
Squashed commits:
[cca27ae] enable jest projects
[e7dc4ce] wip
[dd3ea7a] run in order
[cf87275] define projects
[69baf16] use specific version
[c880cfa] ignore generator test
[e9e0fd3] update travis
[4bbb3c0] update travis
[d63118b] update travis config
[267fe08] remove config
[44883e8] update travis
[40580c3] reduce node version
[59b6add] update ignore rule
[fbbd03c] update readme
[b299dfe] remove script
[a988933] fix test and lint
[95b7d5b] try install and not install
[f4583b4] remove unnecessary files
[c735329] remove eslint-config-xo
[49b3106] pass lint and test
[56c6def] remove git and readd
@kristw kristw added reviewable Ready for review and removed WIP Work in progress labels Nov 7, 2018
Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for all the yak shaving and build debugging for this! had a couple of small comments, could fix the README post-merge if you're anxious to get it in.

👯

]
],
"jest": {
"testPathIgnorePatterns": [
Copy link
Contributor

Choose a reason for hiding this comment

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

can you tell if this gets added to the ones generated beemo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The jest.config.js generated by beemo doesn't have this field by default.
This was added to avoid running the generator tests twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I wasn't clear, just wanted to see if this ignore pattern is added to or over-writes the testPathIgnorePatterns generated by beemo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested removing the field from package.json and jest.config.js remove the field completely. Probably doesn't have that in base.

I tried adding something to package.json's testMatch yesterday and that gets added to the jest.config.js along with the one coming from beemo

cd superset-ui/packages
mkdir superset-ui-new-package
cd superset-ui-new-package
yo @superset-ui/superset
Copy link
Contributor

Choose a reason for hiding this comment

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

is this line correct? not @superset-ui/generator-superset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is correct. yo xxx looks for global npm packages (or npm link'ed package) named generator-xxx.

You will notice all yeoman generator package names start with generator-

This is why I am tempted to name this package generator-superset without @superset-ui so we can call yo superset, but that will not published under the npm org.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah got it, thanks for the explanation 👍

I think using the @superset-ui namespace would still be good.

First, install [Yeoman](http://yeoman.io) and `generator-superset` using [npm](https://www.npmjs.com/) (we assume you have pre-installed [node.js](https://nodejs.org/)).

```bash
npm install -g yo
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. is this recommended vs adding yo as a dep and getting it when you install this package?
  2. also wondering if we should suggest they just run npm run bootstrap in the root, and then @superset-ui/generator-superset + its deps will be installed?
  3. if we do keep the separate installs, do we need them to be global packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1/3) This is the recommended way as there should be single yo client. and yo looks for generator in the global scope.
http://yeoman.io/learning/index.html

  1. This README was written in a way that this packaged can be used independent from this repo. I envision this generator can be extended to have multiple subgenerator for scaffolding independent chart plugin repo, etc. It will be more convenient to install the generator package alone without cloning this monorepo only to npm link the generator.

packages/generator-superset/test/app.test.js Show resolved Hide resolved
@kristw kristw requested a review from a team as a code owner November 7, 2018 19:12
@kristw kristw merged commit 8968cbc into master Nov 8, 2018
@kristw kristw deleted the kristw-generator branch November 8, 2018 00:03
@kristw kristw added this to the v0.6.0 milestone Nov 13, 2018
@kristw kristw added reviewable Ready for review and removed reviewable Ready for review labels Nov 13, 2018
@kristw kristw removed the reviewable Ready for review label Nov 15, 2018
@kristw kristw added the #enhancement New feature or request label Dec 6, 2018
kristw added a commit that referenced this pull request Apr 17, 2020
* fix: labels for many bars in bar chart

* fix: nvd3 bar labels

* docs: update example
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants