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

Add param labels #519

Merged
merged 5 commits into from
Feb 25, 2023
Merged

Add param labels #519

merged 5 commits into from
Feb 25, 2023

Conversation

spapas
Copy link
Contributor

@spapas spapas commented Dec 23, 2022

This implements #486.

@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2022

Codecov Report

Merging #519 (3d78a80) into master (b1d3997) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
+ Coverage   96.55%   96.58%   +0.02%     
==========================================
  Files          52       52              
  Lines        2238     2255      +17     
==========================================
+ Hits         2161     2178      +17     
  Misses         77       77              
Impacted Files Coverage Δ
explorer/tests/test_utils.py 100.00% <ø> (ø)
explorer/views/utils.py 100.00% <ø> (ø)
explorer/models.py 98.41% <100.00%> (+0.03%) ⬆️
explorer/tests/test_models.py 100.00% <100.00%> (ø)
explorer/tests/test_views.py 100.00% <100.00%> (ø)
explorer/utils.py 88.88% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@spapas
Copy link
Contributor Author

spapas commented Jan 18, 2023

Hello friends! Any idea when this will be merged or if it may need some changes to be ready? This is a small change and shouldn't break anything. I'm already using my branch on my project and works great and would really like to see this merged on main to avoid adding the github branch on my requirements.

The only thing that may be needed is that if you are embedding queries in your views (see this issue #485 ). You'll need to change the input label and value similar to how they have been implemented in the internal templates. See the diffs on explorer/templates/explorer/params.html (https://github.com/groveco/django-sql-explorer/blob/3d78a805d428cf4a6e5f10a38b1048a9f9b7496a/explorer/templates/explorer/params.html#L6)

But this change would be done in any case to take advantage of the labels. Also I'm not really sure how many people are using this feature since the #485 didn't get any traction :(

Thank you!

@marksweb
Copy link
Collaborator

@spapas Sorry - I'd missed this PR.

I'm not using the app like this, but can at least take a look at the code and see it makes sense.

It's sometimes a bit frustrating that users aren't more involved on here, because the app does get used by quite a lot of people;
https://pepy.tech/project/django-sql-explorer

@spapas
Copy link
Contributor Author

spapas commented Jan 18, 2023

@marksweb thank you!

Well no matter how you use the app I think that adding labels to your parameters will improve the use experience. Especially if you want to have nice non-english labels, i.e here's a query I use this with:

select aa.id, aa.created_on,
aa.dynamic_data->>'date' as "Ημερμηνία",
aa.dynamic_data->>'authority' as "Υπηρεσία",
aa.dynamic_data->>'mail' as "Email",
aa.dynamic_data
from apps_app aa 
left join apptypes_apptype aat on 
aa.app_type_id = aat.id
where aat.id = 8
and aa.dynamic_data->>'mail' like "%$$email|Email$$%"
and aa.dynamic_data->>'authority' like "%$$authority|Υπηρεσία$$%"

i prefer seeing "Υπηρεσία" instead of "authority" :)

@spapas
Copy link
Contributor Author

spapas commented Feb 12, 2023

Hey @marksweb any news about the status of this PR ?

Thank you!

@marksweb
Copy link
Collaborator

@spapas I'm waiting for pypi to merge the django 4.2 classifier to get a release together. This will go into that.

@marksweb marksweb merged commit fb6f1f5 into explorerhq:master Feb 25, 2023
@spapas spapas mentioned this pull request Mar 5, 2023
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