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

Fix: Docstrings converted to single-line #427

Merged
merged 2 commits into from
Jul 26, 2021
Merged

Conversation

marcorichetta
Copy link
Contributor

Description

These docstrings fits in a single line according to PEP8.

Reference: https://deepsource.io/gh/scanapi/scanapi/issue/FLK-D200/occurrences

Motivation behind this PR?

Solve DeepSource issues.

What type of change is this?

Fix

Checklist

  • I have added a changelog entry / my PR does not need a new changelog entry. Instructions.
  • I have added/updated unit tests. Instructions.
  • New and existing unit tests pass locally with my changes. Instructions
  • I have self-documented code my changes by adding docstring(s) and comment(s). Instructions
  • Current PR does not significantly decrease the code coverage and docstring coverage.
  • My code follows the style guidelines of this project.
  • I have run ScanAPI locally and manually tested my changes. Instructions.

Issue

Closes #411

- Docstrings fits in a single line according to PEP8.
- Reference https://deepsource.io/gh/scanapi/scanapi/issue/FLK-D200/occurrences

Fixes #411
@github-actions
Copy link

@marcorichetta your pull request is missing a changelog!

@codecov-commenter
Copy link

Codecov Report

Merging #427 (672e0fc) into main (35ef8d9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #427   +/-   ##
=======================================
  Coverage   95.21%   95.21%           
=======================================
  Files          22       22           
  Lines         710      710           
=======================================
  Hits          676      676           
  Misses         34       34           
Impacted Files Coverage Δ
scanapi/__main__.py 96.66% <ø> (ø)
scanapi/config_loader.py 92.30% <ø> (ø)
scanapi/exit_code.py 100.00% <ø> (ø)

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 35ef8d9...672e0fc. Read the comment docs.

@marcorichetta marcorichetta marked this pull request as ready for review July 24, 2021 19:25
@marcorichetta marcorichetta requested review from a team as code owners July 24, 2021 19:25
@Pradhvan
Copy link
Member

Pradhvan commented Jul 25, 2021

@marcorichetta Everything else LGTM 👍🏾 can you just squash the commit into one ?

@marcorichetta
Copy link
Contributor Author

@Pradhvan I squashed commits on merge via github Squash and merge but never made it on the same branch.

After reading from Gitlab docs, I was able to squash them but I'm not sure of the final step:

If you haven’t pushed your commits to the remote branch before rebasing, push your changes normally. If you had pushed these commits already, force-push instead.

Is it correct to go with force-push or should I make a new branch?

@Pradhvan
Copy link
Member

Pradhvan commented Jul 26, 2021

@marcorichetta yes, you would need to force push after squashing your commit into one.

The reason we do this is so that the master branch commits have a cleaner commit history as your change can be easily tracked with one commit rather than having two.

Let me know if you need any help with anything.

Copy link
Member

@camilamaia camilamaia left a comment

Choose a reason for hiding this comment

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

🌟

@camilamaia camilamaia merged commit ccfb432 into main Jul 26, 2021
@camilamaia camilamaia deleted the 411-one-line-docstrings branch July 26, 2021 14:23
@camilamaia
Copy link
Member

Noooooo, I've merged it, sorry! My bad 😢

@marcorichetta
Copy link
Contributor Author

The reason we do this is so that the master branch commits have a cleaner commit history as your change can be easily tracked with one commit rather than having two.

@Pradhvan Thanks for the explanation!

I thnk @camilamaia merged the two commits with the Squash and merge option because on main there is only one commit => ccfb432

That said, is there an advantage on squashing locally vs squash on merge? I think the latter is easier both for contributor and for mantainer.

@Pradhvan
Copy link
Member

Pradhvan commented Jul 29, 2021

@marcorichetta yes the latter is an easier option but usually gives out a commit history that is definitely not clean.

For example, this is what the master branch's commit message looks like when it was squashed and merged it.

 Fix: Docstrings converted to single-line (#427)

* Fix: Docstrings converted to single-line

- Docstrings fits in a single line according to PEP8.
- Reference https://deepsource.io/gh/scanapi/scanapi/issue/FLK-D200/occurrences

Fixes #411

* Fix: doc line need to be <= 80 characters

On the other hand, if we had squashed and merged it into a single commit. We would have the liberty to make this message a bit cleaner and more understandable for others who were not involved with the PR. Something like this

 Fix: Docstrings converted to single-line (#427)

* Fix: Docstrings converted to single-line

- Updated `scanapi/__main__.py`, `scanapi/config_loader.py`, `scanapi/exit_code.py` one-line docstring 
to fit on one line with quotes.

Fixes #411

The latter commit message helps in keeping a clean commit history so others know what has been merged with master just by looking at the last commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix documentation issues mentioned in static analysis
4 participants