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

centralize converters #527

Merged
merged 10 commits into from
Oct 18, 2024
Merged

centralize converters #527

merged 10 commits into from
Oct 18, 2024

Conversation

iamdamion
Copy link
Contributor

This PR will (eventually) close #442

  • centralized converters to a single yaml file.
  • updated the converters index.md file to now populate all tables there instead of each in a subfile/subpage.
  • put some placeholder text for each table.

Would like feedback on this draft, please.
Also, unsure why the subfile/subpages (eg converters/eeg.md) left behind "unknown" entries in the table of contents when deleted. How do I clear those table of contents entries. Ideas? @Remi-Gau @bendhouseart ?

@iamdamion iamdamion added this to the Fourth Eval GSOD milestone Oct 10, 2024
Copy link
Contributor

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

Some minor changes required.

The most annoying would sorting the covnerters alphabetically. I think we can maybe add that as some sort of mini validation in another PR: so maybe tackle the easy things first and ping me when you are done with those.

data/tools/converters.yml Show resolved Hide resolved
templates/converters_table_md.jinja Outdated Show resolved Hide resolved
templates/converters_table_md.jinja Outdated Show resolved Hide resolved
templates/converters_table_md.jinja Show resolved Hide resolved
data/tools/converters.yml Outdated Show resolved Hide resolved
data/tools/converters.yml Show resolved Hide resolved
data/tools/converters.yml Show resolved Hide resolved
data/tools/converters.yml Show resolved Hide resolved
Copy link
Contributor

@bendhouseart bendhouseart left a comment

Choose a reason for hiding this comment

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

Agreeing with most of Remi's comments, but less particular about their execution happening now. LGTM

Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

looks fine, just spotted some changes that should be made on a tool. Damion can you please incorporate?

data/tools/converters.yml Outdated Show resolved Hide resolved
data/tools/converters.yml Outdated Show resolved Hide resolved
data/tools/converters.yml Show resolved Hide resolved
iamdamion and others added 5 commits October 17, 2024 09:45
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
Co-authored-by: christinerogers <christinerogers@users.noreply.github.com>
Co-authored-by: christinerogers <christinerogers@users.noreply.github.com>
Co-authored-by: christinerogers <christinerogers@users.noreply.github.com>
@iamdamion iamdamion marked this pull request as ready for review October 17, 2024 16:51
- tools/converters/meeg.md
- tools/converters/physio.md
- tools/converters/others.md
- tools/converters.md
Copy link
Contributor

@Remi-Gau Remi-Gau Oct 18, 2024

Choose a reason for hiding this comment

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

This the main change.
The rest is "cosmetic".

@Remi-Gau
Copy link
Contributor

seems that our markdown linting does not raise warnings as error

will fix in a separate PR

@Remi-Gau Remi-Gau merged commit cb618b7 into main Oct 18, 2024
4 of 5 checks passed
@Remi-Gau Remi-Gau deleted the 442-centralize-converters branch October 18, 2024 09:57
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.

4 participants