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.

Sign up for GitHub

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

Jump to bottom

sorting output should be done in the CLI #19

Open
boneskull opened this issue Jul 15, 2019 · 2 comments
Open

sorting output should be done in the CLI #19

boneskull opened this issue Jul 15, 2019 · 2 comments
Assignees
@boneskull
Labels
bug Something isn't working help wanted Extra attention is needed package-cli Involving cli package package-core Involving core package

Comments

@boneskull
Copy link
Contributor

boneskull commented Jul 15, 2019 •
edited
Loading

Currently, sorting is done in the core API (toReportFromObject), which doesn't make a lot of sense:

  1. sorting must concatenate the stream to an array, which means there's a bottleneck
  2. internally, sorting does not matter
  3. sorting the output only has any bearing on the order of messages output by the inspector
  4. thus, sorting should be done JIT, which would be JUST BEFORE the formatter gets the data
  5. and this should happen in the CLI
@boneskull boneskull added bug Something isn't working package-cli Involving cli package package-formatters package-core Involving core package labels Jul 15, 2019
@boneskull boneskull self-assigned this Jul 15, 2019
boneskull added a commit that referenced this issue Jul 15, 2019
@boneskull
fix(inspector): correct filepath cross-references in aggregate messag…
a707642 
…es; closes #18

- do not hardcode `spec` in `.mocharc.js`; it's impossible to execute a single test file if it's there
- add sourcemaps to Rollup config for debugging
- fix "way too many subscriptions" to `Observable<Report>` instances
- use `_.coerceToArray()` in a couple places
- put `NO_FILEPATH` and `MULTIPLE_FILEPATHS` constants where they should be
- import `gte` and `__` (placeholder) from `lodash/fp`
- sort _before_ instantiating the `Report` object, not after (it will be moved anyway; see #19)
- don't use hardcoded JSON in JSON formatter test
- add some debugs
- fix bad debug namespace in `rule-config.js`
- refactor some logic into a new `Message` class in the inspector
- rename `context` to `report` anywhere NOT in a Rule impl
- remove unused `createReportWithFilepath()`
- don't refuse to instantiate a `Report` from a `Report`
- actually create a `Report` when inspecting (test harness)
- remove dupe of `library-mismatch.spec.js`
boneskull added a commit that referenced this issue Jul 15, 2019
@boneskull
fix(inspector): correct filepath cross-references in aggregate messag…
03fc70a 
…es; closes #18

- do not hardcode `spec` in `.mocharc.js`; it's impossible to execute a single test file if it's there
- add sourcemaps to Rollup config for debugging
- fix "way too many subscriptions" to `Observable<Report>` instances
- use `_.coerceToArray()` in a couple places
- put `NO_FILEPATH` and `MULTIPLE_FILEPATHS` constants where they should be
- import `gte` and `__` (placeholder) from `lodash/fp`
- sort _before_ instantiating the `Report` object, not after (it will be moved anyway; see #19)
- don't use hardcoded JSON in JSON formatter test
- add some debugs
- fix bad debug namespace in `rule-config.js`
- refactor some logic into a new `Message` class in the inspector
- rename `context` to `report` anywhere NOT in a Rule impl
- remove unused `createReportWithFilepath()`
- don't refuse to instantiate a `Report` from a `Report`
- actually create a `Report` when inspecting (test harness)
- remove dupe of `library-mismatch.spec.js`
boneskull added a commit that referenced this issue Jul 15, 2019
@boneskull
fix(inspector): correct filepath cross-references in aggregate messag…
dd81537 
…es; closes #18

- do not hardcode `spec` in `.mocharc.js`; it's impossible to execute a single test file if it's there
- add sourcemaps to Rollup config for debugging
- fix "way too many subscriptions" to `Observable<Report>` instances
- use `_.coerceToArray()` in a couple places
- put `NO_FILEPATH` and `MULTIPLE_FILEPATHS` constants where they should be
- import `gte` and `__` (placeholder) from `lodash/fp`
- sort _before_ instantiating the `Report` object, not after (it will be moved anyway; see #19)
- don't use hardcoded JSON in JSON formatter test
- add some debugs
- fix bad debug namespace in `rule-config.js`
- refactor some logic into a new `Message` class in the inspector
- rename `context` to `report` anywhere NOT in a Rule impl
- remove unused `createReportWithFilepath()`
- don't refuse to instantiate a `Report` from a `Report`
- actually create a `Report` when inspecting (test harness)
- remove dupe of `library-mismatch.spec.js`
@boneskull
Copy link
Contributor Author

also, IMO sorting is out-of-scope for non-CLI usage

@boneskull boneskull changed the title sorting output should be done at the formatter level sorting output should be done in the CLI Jul 17, 2019
@boneskull boneskull added the help wanted Extra attention is needed label Feb 5, 2020
@boneskull
Copy link
Contributor Author

to do this, you'd need to pull any sort-related anything out of the inspect fn in core/src/observable.js and make it part of the stream in cli/src/commands/inspect.js.
there's a sort() in common/src/observable.js helper fn that may help. if it's not used, remove it.

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

@boneskull boneskull

Labels
bug Something isn't working help wanted Extra attention is needed package-cli Involving cli package package-core Involving core package
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@boneskull