FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fvirga/new query route by francescov1 · Pull Request #1538 · diffgram/diffgram · GitHub

/ diffgram Public

fvirga/new query route - #1538

Open
francescov1 wants to merge 13 commits into
masterfrom
fvirga/new-query-route
ConversationCommits (13)ChecksFiles changed
Open

fvirga/new query route#1538
francescov1 wants to merge 13 commits into
masterfrom
fvirga/new-query-route

Conversation

francescov1 commented Aug 28, 2023

Copy link
Copy Markdown
Contributor

See #1536 for relevant comments & context (same diff, just created on a fork instead of direct on repo)

francescov1 marked this pull request as ready for review August 28, 2023 21:10

github-actions Bot commented Aug 28, 2023 •
edited
Loading

Copy link
Copy Markdown

Unit Test Results

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit ea19c51. ± Comparison against base commit 0df190f.

♻️ This comment has been updated with latest results.

francescov1 commented Aug 31, 2023
View reviewed changes
Comment thread default/methods/account/plan/usage_cycle.py
@@ -260,7 +260,7 @@ def determine_billable_directory_file_and_instance(

francescov1 Aug 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

See #1536 for comments about the diffs

anthony-chaudhary reviewed Sep 21, 2023
View reviewed changes
Comment thread shared/query_engine/query_creator.py
self.member = member
self.directory = directory
# Additional security check just for sanity
Project_permissions.by_project_core(

anthony-chaudhary Sep 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is a duplicate.
Context is that this is called later in SqlAlchemyQueryExecutor

Part of larger goal of aligning permissions into the same place

anthony-chaudhary Sep 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Also that in general we are trying to put more validation at execution stage rather then prep/build stage (e.g dataset IDs which rely on the query string builder to format it first)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anthony-chaudhary anthony-chaudhary left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL