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

Refactor agentql repo #97

Merged
merged 11 commits into from
Nov 11, 2024
Merged

Refactor agentql repo #97

merged 11 commits into from
Nov 11, 2024

Conversation

frankfeng98
Copy link
Contributor

@frankfeng98 frankfeng98 commented Nov 6, 2024

The merging of this PR will wait and coordinate with Doc website release, as we will need to update example links on website as well.

What this PR does:

(1) Refactor the structure of the folder to align with this:

https://tinyfishgroup.slack.com/archives/C07NPTE4UNA/p1730482953278499

image

(2) Adjust the Github action workflow so that it will now run separately for Python and JS.

(3) Rename Python files to main.py.

The reason I put these together and this PR does multiple things is because it will make it easier to coordinate with changes on documentation website.

@rachelnabors
Copy link
Contributor

Two questions!

  1. Should it be:
  • Examples
  • JS
  • Python

or

  • JS Examples
  • Python Examples
  1. Does the core README need updating? I don't think I see it...

Copy link
Contributor

@paveldudka paveldudka left a comment

Choose a reason for hiding this comment

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

More of a pet peeve, but lets not capitalize folder names.
js + python

@frankfeng98
Copy link
Contributor Author

Two questions!

  1. Should it be:
  • Examples
  • JS
  • Python

or

  • JS Examples
  • Python Examples
  1. Does the core README need updating? I don't think I see it...

Regarding the first question, I think we previously aligned that we would go with Examples and then js, python subfolders. But if we want to change it, I could also make the adjustments.

And I have updated README in the latest commit. Thank you for flagging it!

Copy link
Contributor

Choose a reason for hiding this comment

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

please don't use capitalized folder names

Copy link
Contributor

Choose a reason for hiding this comment

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

does it have to be "javascript-sdk"? So verbose. Can it be just "js"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I think javascript-sdk is already removed from this repo with this PR. The reason it's showing up is because the files it contained were deleted (along with this folder).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual js folder is under examples/js

Copy link
Contributor

@colriot colriot left a comment

Choose a reason for hiding this comment

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

pls, check out comments about CLI and names

],
"python.analysis.typeCheckingMode": "standard",
"python.analysis.typeCheckingMode": "basic",
Copy link
Contributor

Choose a reason for hiding this comment

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

why and what's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial thinking is that "basic" is more lenient in terms of reporting static type warnings/errors and might make it easier for others to contribute. But on a second thought, we should probably still make it standard to ensure code quality -- I will revert the change

Copy link
Contributor

Choose a reason for hiding this comment

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

might be better to name them "template_*", using suffixes. but in any case, we'll need to address one thing: CLI tool will fail downloading the template. Can you double check, do we download from main of from a specific commit? cc @frankfeng98

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could download from a specific branch, but probably not specific commit. Therefore, we need to update and release CLI (with links pointing to new file path) and merge this PR roughly at the same time.

Copy link
Contributor

@rachelnabors rachelnabors left a comment

Choose a reason for hiding this comment

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

Ok, let's stick with /examples. I'm ready to approve!

@rachelnabors rachelnabors merged commit fe93cbf into main Nov 11, 2024
2 checks passed
@rachelnabors rachelnabors deleted the refactor_agentql_repo branch November 11, 2024 15:44
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.

4 participants