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

Upgrade Dredd Transactions and refactor how annotations are processed #1351

Merged
merged 3 commits into from
May 10, 2019

Conversation

honzajavorek
Copy link
Contributor

@honzajavorek honzajavorek commented May 6, 2019

🚀 Why this change?

Dredd Transactions contain a lot of new fixes and features (not only) in OAS3 support. There's more and more issues on Dredd which are filed only because Dredd is behind in what has been already developed.

Dredd is behind because new versions of the parsing toolchain caught me off guard during refactoring rather larger portions of the Dredd codebase. This should be the last missing piece, which will address pending issues around annotation line/column numbers and will upgrade Dredd Transactions in the process to address outstanding OAS3 issues.

📝 Related issues and Pull Requests

✅ What didn't I forget?

  • To write docs
  • To write tests
  • To put Conventional Changelog prefixes in front of all my commits and run npm run lint

@honzajavorek honzajavorek force-pushed the honzajavorek/upgrade-dt branch 2 times, most recently from 20b9d7b to 13c3393 Compare May 9, 2019 15:12
Copy link
Contributor

@artem-zakharchenko artem-zakharchenko left a comment

Choose a reason for hiding this comment

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

I've only had minor comments. Otherwise approved!

const annotationsError = handleRuntimeProblems(apiDescriptions, this.logger);
if (annotationsError) { callback(annotationsError, this.stats); return; }
const loggerInfos = toLoggerInfos(apiDescriptions);
// FIXME: Winston 3.x supports calling .log() directly with the loggerInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

We can reference #1225 here to keep track of the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

if (origin.resourceGroupName) { segments.push(origin.resourceGroupName); }
if (origin.resourceName) { segments.push(origin.resourceName); }
if (origin.actionName) { segments.push(origin.actionName); }
if (origin.exampleName) { segments.push(origin.exampleName); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this is copy-pasted, but you can also do a slightly shorter syntax:

const segments = [
  origin.apiName,
  origin.resourceGroupName,
  origin.resourceName,
  origin.actionName,
  origin.exampleName,
].filter(Boolean).join(' > ')

Apart from shorter declaration it also reduces the function's complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good suggestion. I'll file it as a DT issue and fix it there. This wile stays copy-pasted and then deleted once DT provides the name directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ApiaryBot
Copy link
Collaborator

🎉 This PR is included in version 11.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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