User defined archive access url (local branch PR)

[francoismg]

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

[dsavchenko]

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

[dsavchenko]

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]

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]

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

[dsavchenko]

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

[francoismg]

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]

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]

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]

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]

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]

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]

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]

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 !

[francoismg]

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]

Actions broken for unknown reason

[dsavchenko]

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]

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]

@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]

@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]

@francoismg any progress on fixing linting?

[francoismg]

@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]

@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]

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]

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]

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

[francoismg]

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]

@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]

@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.

[volodymyrss]

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