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

feat: add initial sql formatting tool #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vaeng
Copy link
Contributor

@vaeng vaeng commented Jan 30, 2024

This file is supposed to end up as a github action at some point to keep our code consistent.

For now let's discuss the changes it makes:

  • split lines at 120 chars
  • change keywords to uppercase
  • change identifiers to lowecase
  • reindent

Details here: https://sqlparse.readthedocs.io/en/latest/api/#formatting-of-sql-statements

@vaeng vaeng added x:action/improve Improve existing functionality/content x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows) labels Jan 30, 2024
@glennj
Copy link
Contributor

glennj commented Jan 30, 2024

Is there an npm-like tool for python?

@vaeng
Copy link
Contributor Author

vaeng commented Jan 30, 2024

Is there an npm-like tool for python?

There are some alternatives.

Copy link
Member

@IsaacG IsaacG left a comment

Choose a reason for hiding this comment

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

Are we going the Python-route for tooling? Should I assume Python and use Python to make exercise generators?

@@ -0,0 +1,104 @@
import argparse
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import argparse
#!/usr/bin/env python
import argparse

)
buffer = []
counter = 1
for line in diff:
Copy link
Member

Choose a reason for hiding this comment

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

Rather than building your own output, do you just want difflib.ndiff()?

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 tested it a bit, but it did not really do what I hoped to see. Maybe my config was off?



def dir_path(path):
if os.path.isdir(path):
Copy link
Member

Choose a reason for hiding this comment

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

pathlib is the modern library for path operations.

safeguard_sqlite_info = []
for line in original.splitlines():
if re.match(r'^[ ]*.|(--)', line):
safeguard_sqlite_info.append("/*TEMP" + line + "TEMP*/")
Copy link
Member

Choose a reason for hiding this comment

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

Does this break on trailing comments? Does this mean any line starting with a space is assumed to be a comment?

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 should escape the dot.
Trailing comments stay trailing with the normal parser, but single-line comments would be added after the last SQL statement, that did not look right for me, so I changed it.



def parse_dir(args):
for subdir, dirs, files in os.walk(args.path):
Copy link
Member

Choose a reason for hiding this comment

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

pathlib.walk() or pathlib.rglob("*") might be easier to use.

pathlib also supports a pathlib.rename() and read_text() etc

@IsaacG
Copy link
Member

IsaacG commented Jan 30, 2024

Is there an npm-like tool for python?

I would suggest pip

@glennj
Copy link
Contributor

glennj commented Jan 30, 2024

Is there an npm-like tool for python?

I would suggest pip

As a relative python newbie, what's the process for installing needed libraries?

  1. run the script
  2. no errors? we're done.
  3. get an error
  4. manually pip install
  5. go to 1

@IsaacG
Copy link
Member

IsaacG commented Jan 30, 2024

For every import statement, the library must either (1) be part of the Standard Library or (2) pip install $library.

@glennj
Copy link
Contributor

glennj commented Jan 30, 2024

I'll hold off on any further python library objections. Perhaps all that's required, once we see how much python tooling this track gets, is a documented list in CONTRIBUTING.md which libraries need installing.

@vaeng
Copy link
Contributor Author

vaeng commented Jan 30, 2024

I'll hold off on any further python library objections. Perhaps all that's required, once we see how much python tooling this track gets, is a documented list in CONTRIBUTING.md which libraries need installing.

We could have a requirements file, that can be fed into pip. This is how many projects ensure that the libs are where they are needed. It is a pretty smooth process in my experience.

I understand, that the dependencies are easy to handle if we use bash only, but I have to admit, that it takes me a lot longer to configure the tooling if I write it in bash. Github actions seem quite capable of running Python script in a dependable manner as well.

For tooling that is probably used by students, I want to keep the requirements low. For the maintenance side, I am open to using the things that bring this track online and ready in the best way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/improve Improve existing functionality/content x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants