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

User defined archive access url (local branch PR) #78

Merged
merged 21 commits into from
Mar 26, 2024

Conversation

francoismg
Copy link
Contributor

Add the possibility for users to use a custom access url in order to query not supported archives

@francoismg francoismg linked an issue Jan 31, 2024 that may be closed by this pull request
@dsavchenko
Copy link
Collaborator

Preview is available https://galaxy.odahub.fr/?tool_id=astronomical_archives_pr78&version=latest

@dsavchenko
Copy link
Collaborator

I fixed this https://github.com/esg-epfl-apc/tools-astro/actions/runs/7724870060/job/21057927546?pr=78
But do we really need such a strict linting?

@francoismg
Copy link
Contributor Author

I fixed this https://github.com/esg-epfl-apc/tools-astro/actions/runs/7724870060/job/21057927546?pr=78 But do we really need such a strict linting?

Ok thanks, I was waiting for feedback to see if there was some changes to do before fixing the linting.

And yes linting is way too strict (more strict than what they use for galaxy core too btw) but i'm not sure we can change it ourselves since it's used to validate things for the toolshed

@dsavchenko
Copy link
Collaborator

dsavchenko commented Feb 1, 2024

This failure is very cryptic. But doesn't this entire workflow duplicate other ones?
@volodymyrss probably you remember where it comes from?

@dsavchenko
Copy link
Collaborator

dsavchenko commented Feb 1, 2024

This one is also a bit cryptic. But probably you understand what's the reason, @francoismg ?

@francoismg
Copy link
Contributor Author

francoismg commented Feb 1, 2024

This one is also a bit cryptic. But probably you understand what's the reason, @francoismg ?

Yes it's because an url from the tool documentation is unavailable at the moment, I will just format it so it doesn't appear like a url during parsing and it will pass the test.

Maybe that will fix the first error too cause if I remember correctly the first workflow success depend on the success of the following ones.

@francoismg
Copy link
Contributor Author

This one is also a bit cryptic. But probably you understand what's the reason, @francois ?

The second one is fixed now, but the first one still fails I will have to check it a bit

@francoismg
Copy link
Contributor Author

The tests are all good when running "planemo lint" locally but the workflow is a bit different here

I saw that in the log too, is it possible that the other tool in the repo are tested alongside this one and causing the error?

linting tools/astropytools/fits2bitmap.xml
Linting tool /home/runner/work/tools-astro/tools-astro/tools/astropytools/fits2bitmap.xml
.. CHECK: Tool defines a name [astropy fits2bitmap].
.. CHECK: Tool defines an id [astropy_fits2bitmap].

@francoismg
Copy link
Contributor Author

Ok The same error is appearing in every pull request so I think that the problem is not linked to that tool or PR

Every recent PR has the same errors :

File "src/lxml/xmlschema.pxi", line 90, in lxml.etree.XMLSchema.init
lxml.etree.XMLSchemaParseError: complex type 'Output': The content model is not determinist., line 5162

and the bits about fits2bitmap

@dsavchenko

@francoismg
Copy link
Contributor Author

I tried to modify the worflow to run it only on some tools and it seems that you get the same error whatever tools you test, also I saw that there's the same error in commits from weeks ago so it looks like the problem is not recent

@francoismg
Copy link
Contributor Author

I was able to fix the error by updating planemo version in environment.yml will check to be sure and then make a PR

@volodymyrss
Copy link
Contributor

I was able to fix the error by updating planemo version in environment.yml will check to be sure and then make a PR

Cool, thanks! I can also see it on preview.

Is it possible to restrict text field pattern? So it only allows URLs..

@volodymyrss
Copy link
Contributor

After looking around in the preview (I really find it useful, much faster than pull, planemo s, etc) I confirm it seems to do what we want. Thanks @francoismg !

image

@francoismg
Copy link
Contributor Author

Still one bug to fix when writing noir lab user chosen url field to the csv file

write bug is fixed, also there's seems to be some differences between regex processing from the xml and with python so the regex are now a bit different but are working correctly

@volodymyrss
Copy link
Contributor

Actions broken for unknown reason

@dsavchenko
Copy link
Collaborator

I already raised this question. There are always problems with this specific action. And it seems to have just a duplicated functionality with the other more elaborated ones.
I suspect, looking at the history, this one was created manually as a first try, but then "official" CI/CD was added to the repo. @volodymyrss please confirm
So we are safe to just remove it completely, I think

@volodymyrss
Copy link
Contributor

I already raised this question. There are always problems with this specific action. And it seems to have just a duplicated functionality with the other more elaborated ones. I suspect, looking at the history, this one was created manually as a first try, but then "official" CI/CD was added to the repo. @volodymyrss please confirm So we are safe to just remove it completely, I think

No idea, I took an example provided, feel free to remove in this PR if it is indeed duplicated.

@dsavchenko
Copy link
Collaborator

dsavchenko commented Mar 19, 2024

@francoismg this tool linting fail is probably again about unresolvable link in description, I'm not sure, could you look on it?

But tests pass!

@francoismg
Copy link
Contributor Author

@francoismg this tool linting fail is probably again about unresolvable link in description, I'm not sure, could you look on it?

But tests pass!

yes looks like a documentation url is not reachable, I will break it so it is not parsed as a url

Also seems that there's one line too long and some whitespaces somewhere, will fix that too

@dsavchenko
Copy link
Collaborator

@francoismg any progress on fixing linting?

@francoismg
Copy link
Contributor Author

@francoismg any progress on fixing linting?

yes the problem is that the regex triggers some errors cause it's treated like a regular string so I will have to replace the regex with some urllib functions and parse the url myself like we mentionned earlier so it won't be the same validation in the xml and the python script. I was working on the jsvis right now but I will make and push the archive tool changes today

@francoismg
Copy link
Contributor Author

@francoismg any progress on fixing linting?

yes the problem is that the regex triggers some errors cause it's treated like a regular string so I will have to replace the regex with some urllib functions and parse the url myself like we mentionned earlier so it won't be the same validation in the xml and the python script. I was working on the jsvis right now but I will make and push the archive tool changes today

or maybe use the raw string thing r'' maybe it will make flake8 understand that it's not escaping blackslash and we could keep the regex, I will try that before switching to urllib

@dsavchenko
Copy link
Collaborator

or maybe use the raw string thing r''

Even regardless flake8, my experience is that it's always better to use raw string for the regex

@francoismg
Copy link
Contributor Author

or maybe use the raw string thing r''

Even regardless flake8, my experience is that it's always better to use raw string for the regex

ok will do that then thanks, it was like that before but I removed it for some reason, I should have kept it

@dsavchenko
Copy link
Collaborator

And for some "line too long" I would've just used #noqa because it really seems too restrictive. Sometimes, following linter suggestions only worsen code readability

f string instead of concat

Co-authored-by: Denys Savchenko <[email protected]>
@francoismg
Copy link
Contributor Author

And for some "line too long" I would've just used #noqa because it really seems too restrictive. Sometimes, following linter suggestions only worsen code readability

Yes it's way too restrictive I have too keep one working version where I dont follow every suggestions cause the linted version is so unpractical

I thought we had to follow every rules to adapat to galaxy standards since it's pushed to the tool shed but ok if we can just make it ignore some things it's great I will add that too where it makes sense

@dsavchenko
Copy link
Collaborator

@volodymyrss I made final polishing so that we have green mark here. I think we can merge right after your review.
@francoismg please protest if you wanted to add something to this PR

@francoismg
Copy link
Contributor Author

francoismg commented Mar 25, 2024

@volodymyrss I made final polishing so that we have green mark here. I think we can merge right after your review. @francoismg please protest if you wanted to add something to this PR

No it's all good I was just going to try to add the raw string for the regex and try the noqa flag but since you did it already it's fine with me thanks.

Copy link
Contributor

@volodymyrss volodymyrss left a comment

Choose a reason for hiding this comment

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

Looks good I think, I also tried it with other URLs I had. Thanks!

@dsavchenko dsavchenko merged commit b77ceb5 into main Mar 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add free field for custom url
4 participants