-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
implements req.getPathName() #3298
base: main
Are you sure you want to change the base?
Conversation
Great PR, however I personally feel the chosen name is quite confusing. I would rather expect to get the current path part of the Request URL than the script location. Maybe we could rename it to something more clear. All the related Feature Requests suggested different names, maybe we take one of those or any other. |
Replying to @nikischin
I agree
Using a part of the request URL may be complicated. Some APIs use path parameters in the request URL, some have query parameters and some may have special characters. req.url should be good if a script requires the request URL. I would prefer if the |
Thanks for your feedback @nikischin, @varunvora For my use case I ended up triming the result of req.getPathName() in script to get relative path + bru name. I think this function could be splitted in two:
base Path is already in:
About include them as properties insted of methods I have no preference but previous approved PRs added methods, so I think it is more probable to get this merge as methods. What do you think? |
I personally don't have any opinion on the implementation, as I did not have the need for this feature yet. However, I guess this is a valuable feature, so I would support it being implemented. I was just trying to recommend renaming it to something more precise. With That's why I was recommending to rename it, maybe we could use |
@tlaloc911 thanks for your proposal! |
hi @helloanoop , what do you thing about this?, your feedback is appreciated |
@tlaloc911 i'm working on a pr to add i just wanted to check if you would be okay with updating the pr if you agree with this pattern
cc: @helloanoop |
@tlaloc911 @nikischin I created a proposal to discuss the API in depth - See #3817 @lohit-bruno I am fairly confident with the |
Description
implements function req.getPathName() in GUI and CLI
Resolves #2480, #1874, #987, #3195
GUI
![image](https://private-user-images.githubusercontent.com/29415755/376108639-aecd86ad-34b3-4830-9656-a16aead97bc8.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk3NTkxNzEsIm5iZiI6MTczOTc1ODg3MSwicGF0aCI6Ii8yOTQxNTc1NS8zNzYxMDg2MzktYWVjZDg2YWQtMzRiMy00ODMwLTk2NTYtYTE2YWVhZDk3YmM4LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE3VDAyMjExMVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTcwMzg5YjcwNTFmOTk3N2EyYWU3NjlmOGE0ZjNjY2Y3MmRhZThlNjFkZGQ5ZDJiNzFiMjM5MWQwZjE3NzNkZjkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.g8cD197zHRwF5gsBdqGQV7rtpIgQuThIndP81ysWPYg)
CLI
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.