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

pulsarify all tools in the repo #281

Closed

Conversation

martenson
Copy link
Contributor

@martenson martenson commented Jan 2, 2025

pulsar needs to explicitly know about the files included in the command, this PR adds this information to all tools in the repo that are missing it (some already had it)

While having a look a this I also found some minor inconsistencies in requirements and syntax, check the later commits for details.

closes #279

FOR CONTRIBUTOR:

  • - I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • - License permits unrestricted use (educational + commercial)
  • - This PR adds a new tool or tool collection
  • - This PR updates an existing tool or tool collection
  • - This PR does something else (explain below)

@martenson martenson requested a review from lecorguille as a code owner January 2, 2025 09:02
Copy link
Contributor

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

Thanks @martenson. Can you maybe come up with a small script to detect those tools. We probably want to add this to more tools.

@martenson
Copy link
Contributor Author

@bgruening done

@martenson martenson changed the title readmsdata: add required files for pulsar pulsarify all tools in the repo Jan 8, 2025
Copy link
Contributor

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

That looks great!

@hechth
Copy link
Contributor

hechth commented Jan 17, 2025

@martenson is there still an issue or do you have some idea why the tests are failing?

@martenson
Copy link
Contributor Author

@hechth I am not aware of any issue and I do not have access to this repo to troubleshoot tests.

@bgruening
Copy link
Contributor

Many tools' linting is failing because we are touching many tools now. Of course, it would be nice to fix those, but I can also understand if you just want to merge it.

@martenson
Copy link
Contributor Author

I took care to not break the linting with my modifications and I do not have the availability to fully re-lint all tools I am fixing here.

@hechth
Copy link
Contributor

hechth commented Jan 20, 2025

@martenson I'll see what I can do to fix tests and linting etc.

@bernt-matthias
Copy link
Contributor

bernt-matthias commented Jan 20, 2025

With #282 CI should run again.

  • tool linting problems can be added to a .lint_skip file, e.g. see here
  • problems with R linting can be fixed using the styler.R tool (from the above PR) .. but deployment should also work without fix

@hechth
Copy link
Contributor

hechth commented Jan 21, 2025

@bernt-matthias I am currently working on fixing the linting and testing, to also give the tools some general update.

@hechth hechth mentioned this pull request Jan 21, 2025
5 tasks
martenson and others added 5 commits January 21, 2025 11:44
in a version roughly from the same timeline
slesh is already part of the macro, not sure if this can faiil sometimes, but this commit should be correct always
@martenson
Copy link
Contributor Author

rebased on top of #282 -- thank you @bernt-matthias !

@bernt-matthias
Copy link
Contributor

Can someone approve the workflow run?

@hechth
Copy link
Contributor

hechth commented Jan 21, 2025

this is superceded by #283

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.

Add required_files to tools/xcms/msnbase_readmsdata.xml
4 participants