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

URLConfig preprocessor #1110

Merged
merged 27 commits into from
Jan 16, 2023
Merged

URLConfig preprocessor #1110

merged 27 commits into from
Jan 16, 2023

Conversation

jmosbacher
Copy link
Contributor

A few small improvements to URLConfigs, similar to #827 but a bit simpler implementation.

  1. Formalizes the URL parsing as a string to AST and add reverse operation
  2. Adds validate method that runs before the config is passed to the plugin.
  3. Adds the ability to register preprocessors that can alter the URL/config before it is hashed.

Preprocessors are passed the config and return a new/same config. They are applied in order of higher precedence first.

@coveralls
Copy link

coveralls commented Nov 10, 2022

Coverage Status

Coverage: 93.529% (-0.4%) from 93.91% when pulling bef1900 on url_config_preprocessor into eb0fe12 on master.

@jmosbacher jmosbacher marked this pull request as ready for review January 12, 2023 11:45
Copy link
Contributor

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

@jmosbacher , looks good. In #827 you had a few tests, can you re-use those examples again?

Should we make the format_url_kwargs a default someday, or is xedocs already planned to use this feature?

Uhm, there are two self.format_url_kwargs methods? L193, L325 🤔

straxen/url_config.py Outdated Show resolved Hide resolved
straxen/url_config.py Outdated Show resolved Hide resolved
straxen/url_config.py Outdated Show resolved Hide resolved
jmosbacher and others added 5 commits January 15, 2023 16:16
Co-authored-by: Joran R. Angevaare <jorana@nikhef.nl>
Co-authored-by: Joran R. Angevaare <jorana@nikhef.nl>
Co-authored-by: Joran R. Angevaare <jorana@nikhef.nl>
@jmosbacher
Copy link
Contributor Author

@JoranAngevaare hmm you are right, now i realize why i left it as a draft for so long. I never got around to finishing polishing it and adding tests... Ill do that now.

@jmosbacher
Copy link
Contributor Author

@JoranAngevaare I added some tests and documentation

@JoranAngevaare
Copy link
Contributor

JoranAngevaare commented Jan 16, 2023

Nice work Yossi, I added a test to assert I understand it well.

Two final requests that I think would be useful for dummies like me:

  • Add a preprossor_descr(cls) like protocol_descr(cls) and
  • show(cls), that shows the overall state preprocessor + protocol (what is registered, what precedence is associated to each preprocessor, where is the code that adds it). We can add the output of that to the autodocs a la plugin tree, which would also solve URL protocols would benifit from autogenerated page #1111. The autodocs I can also do and/or make a separated PR for it

@jmosbacher
Copy link
Contributor Author

Good idea. I added methods for the preprocessor as well as a print_summary method that prints both

@jmosbacher
Copy link
Contributor Author

@JoranAngevaare did you want to already add the autodoc feature to this PR before its merged?

@JoranAngevaare
Copy link
Contributor

@jmosbacher let's do it in a seperate PR, nice work Yossi, thanks!

@JoranAngevaare JoranAngevaare merged commit 68d56ba into master Jan 16, 2023
@JoranAngevaare JoranAngevaare deleted the url_config_preprocessor branch January 16, 2023 12:07
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