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

Core: Introduce ProcessingQueue in internals #1121

Merged
merged 1 commit into from
Mar 24, 2017

Conversation

trentmwillis
Copy link
Member

Currently, the logic for processing the queue of tests is scattered. This pulls the two core pieces into a new construct ProcessingQueue with an interface that more clearly communicates what process and synchronize used to do.

In other words:

  • process() -> ProcessingQueue.advance(): Moves the queue forward
  • synchronize() -> ProcessingQueue.add(): Adds a function to the queue

This is being done preemptively to support the "watch" feature in the forthcoming CLI. We'll need to refactor a portion of how the queue is processed in order to properly abort test runs when watched files change.

Copy link
Contributor

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Just one indentation issue otherwise LGTM. Feel free to dismiss this review when indentation is fixed if I'm slow to follow up.

generateHash,
now,
objectType
} from "./utilities";
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent indentation here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! We should probably turn on the eslint rule for indentation...and I should probably update my editor workspace to use tabs in QUnit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Re: ESLint rule for indentation: One bit of the jQuery style guide, which the ESLint indent rule doesn't yet support, stymies this.

Specifically: Things that look like a top-level whole-file IIFE or function wrapper, but aren't pure IIFEs, are supposed to have an indent level of zero for contained statements. For example, AMD define callbacks, or assignments to module.exports that are functions, or IIFEs that follow the flexible factory pattern (the factory selector/"intro" code needs one indent level, the actual code needs zero). All of those are use cases not yet supported by ESLint.

That said, we could choose to activate the rule anyway and just accept that for this project, all function bodies will be indented by one tab. I don't think that's the end of the world to be completely honest. And both when we activate the rule and if we consume an upgraded version of the rule which supports our special function indentation preferences, we can use eslint --fix to fix the existing problems fairly easily.

I'll open an issue re: indent rule.

@trentmwillis
Copy link
Member Author

@platinumazure updated, thanks for the review!

@trentmwillis trentmwillis mentioned this pull request Mar 24, 2017
1 task
Copy link
Contributor

@platinumazure platinumazure 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!

@trentmwillis trentmwillis merged commit faae7a0 into qunitjs:master Mar 24, 2017
@trentmwillis trentmwillis deleted the processing-queue branch March 2, 2018 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants