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

Fixed typo in README and added instructions on how to virtualenv #457

Merged

Conversation

dnshio
Copy link
Contributor

@dnshio dnshio commented Oct 4, 2020

This looks like a really great open source project. I'd like to contribute to its development so I started reading your docs. I noticed a couple of small issues there so here's a PR to fix them.

With regards to instructions on virtualenv, I'm open to feedback on whether I should include more detail. Also, my improvement only recommends python's venv module rather than the independent virtualenv package. I believe in recommending a single tool and and python's venv module is best because it doesn't required additional installation.

Let me know how I could improve this PR.

@codecov
Copy link

codecov bot commented Oct 4, 2020

Codecov Report

Merging #457 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #457   +/-   ##
=======================================
  Coverage   91.74%   91.74%           
=======================================
  Files          30       30           
  Lines        4654     4654           
=======================================
  Hits         4270     4270           
  Misses        384      384           
Flag Coverage Δ
#py35 91.61% <ø> (ø)
#py36 91.61% <ø> (ø)
#py37 91.61% <ø> (ø)
#py38 91.56% <ø> (ø)
#py39 91.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 7d68d01...9cd0bc3. Read the comment docs.

@NiallRees
Copy link
Member

This looks like a really great open source project. I'd like to contribute to its development so I started reading your docs. I noticed a couple of small issues there so here's a PR to fix them.

With regards to instructions on virtualenv, I'm open to feedback on whether I should include more detail. Also, my improvement only recommends python's venv module rather than the independent virtualenv package. I believe in recommending a single tool and and python's venv module is best because it doesn't required additional installation.

Let me know how I could improve this PR.

Yes Dinesh! I'm personally a big pipenv fan, have you used it and do you think it could be a good choice here rather than venv?

@@ -13,7 +13,7 @@
[![CircleCI](https://img.shields.io/circleci/build/gh/sqlfluff/sqlfluff/master?style=flat-square&logo=CircleCI)](https://circleci.com/gh/sqlfluff/sqlfluff/tree/master)
[![ReadTheDocs](https://img.shields.io/readthedocs/sqlfluff?style=flat-square&logo=Read%20the%20Docs)](https://sqlfluff.readthedocs.io)

Bored of not having a good SQL linter that works with whichever dialiect you're
Bored of not having a good SQL linter that works with whichever dialect you're
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed this as well the other day, thanks for fixing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😄 I thought this was intentionally left there for new comers!

@pwildenhain
Copy link
Member

pwildenhain commented Oct 4, 2020

Yes Dinesh! I'm personally a big pipenv fan, have you used it and do you think it could be a good choice here rather than venv?

Personally, my vote is for venv 🤚 though if we start to run into tricky dependency issues, I could see opting for pipenv instead. poetry is also worth considering if you're going to consider pipenv since it's built specifically for packages.

@NiallRees
Copy link
Member

Out of interest are you able to merge this now I've approved it @dnshio ? Or do I need to

@dnshio
Copy link
Contributor Author

dnshio commented Oct 4, 2020

Yes Dinesh! I'm personally a big pipenv fan, have you used it and do you think it could be a good choice here rather than venv?

Personally, my vote is for venv 🤚 though if we start to run into tricky dependency issues, I could see opting for pipenv instead. poetry is also worth considering if you're going to consider pipenv since it's built specifically for packages.

Have used pipenv for a couple of projects and I ended up ditching it because it became more hassle than what it was worth. Maybe I didn't see the true power of it. But I grew to appreciate the simplicity of having either a setup.py or a requirements.txt with pinned versions (point versions) and let the client decide what virtual environment they want to use.

@dnshio
Copy link
Contributor Author

dnshio commented Oct 4, 2020

Out of interest are you able to merge this now I've approved it @dnshio ? Or do I need to

I do not have permission to merge. Would be great if you could :)

@NiallRees NiallRees merged commit 31bc3c2 into sqlfluff:master Oct 4, 2020
@dnshio dnshio deleted the improvement/documentation-improvements branch October 5, 2020 14:30
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.

3 participants