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

Jump to bottom

Inline mode #9

Merged
merged 6 commits into from Apr 2, 2018
Merged

Inline mode #9

merged 6 commits into from Apr 2, 2018

Conversation

sunshinejr
Copy link
Collaborator

@sunshinejr sunshinejr commented Mar 4, 2018 •
edited

(This needs to wait before danger-swift#56 is merged, though) merged now 😉

Todo:

  • Add documentation
  • Add Changelog entry

ashfurrow
ashfurrow approved these changes Mar 5, 2018
View reviewed changes
Copy link
Owner

@ashfurrow ashfurrow left a comment

Choose a reason for hiding this comment

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

Cool, nice approach with DI on those functions 👍

@sunshinejr sunshinejr changed the title [WIP] Inline mode Inline mode Mar 24, 2018
@sunshinejr
Copy link
Collaborator Author

sunshinejr commented Mar 31, 2018 •
edited

Update: So the Danger JS and Danger Swift inline mode was merged! Danger JS is already released and @orta is playing with Danger Swift to get all the things working for the release as well. Once Danger Swift is released, I'm gonna add documentation and we could use a final review for this one!

Update2: IT'S GREEN EVERYBODY. Writing docs as we speak.

@sunshinejr
Copy link
Collaborator Author

sunshinejr commented Mar 31, 2018 •
edited

@ashfurrow okay, Danger Swift is released, I've added Changelog entry & documentation. Should be ready for the final review 😉

Edit: or wait, seems like there is one tiny bug to fix 😄
zrzut ekranu 2018-04-01 o 01 08 48

Edit2: OMG, it's working. See Harvey#17. Basically SwiftLint always returned absolute path and because we had relative ones, we could swap it after generating Violations.

ashfurrow
ashfurrow approved these changes Apr 2, 2018
View reviewed changes
Copy link
Owner

@ashfurrow ashfurrow left a comment

Choose a reason for hiding this comment

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

Looks great! Just one small note, not a blocker. Merge when you're ready 👍

README.md
@@ -31,6 +31,14 @@ SwiftLint.lint()

That will lint the created and modified files

#### Inline mode

If you want the lint result shows in diff instead of comment, you can use inline_mode option. Violations that out of the diff will show in danger's fail or warn section.
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any Danger docs or code we could link to from here, to give more context about what inline mode is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, good idea. I will add a link to the release notes of Danger JS, it's really well written 👍

@sunshinejr sunshinejr merged commit 590a160 into ashfurrow:master Apr 2, 2018
@sunshinejr sunshinejr deleted the inline branch April 2, 2018 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashfurrow ashfurrow ashfurrow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sunshinejr @ashfurrow