Skip to content

Support --subjects as csv #274

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abdulazimRabie
Copy link
Contributor

@abdulazimRabie abdulazimRabie commented Apr 8, 2025

Fix : #271

Approach:

  • Add --subjects to parameterList as a CLI option
  • Verify that passed subjects are valid and match with rootCategories and cats
  • Select both cats and rootCategories values based on passed values.

e.g : --subjects 'math, physics'

  • cats : {4: 'physics', 15: 'math'}
  • rootCategories : [Math, Physics]
  • simTree (if en,mo passed as includeLangauges):
en: {
    'atomic-interactions': [ 'Physics' ],
    'balancing-act': [ 'Physics', 'Math' ],
    .....
}

 mo: {
    'atomic-interactions': [ 'Physics' ],
    'balancing-act': [ 'Physics', 'Math' ],
    ......
}

Note:

  • I have added formatcsv as a util function for all future csv parameters.

@kelson42 kelson42 requested a review from benoit74 April 8, 2025 17:26
@abdulazimRabie abdulazimRabie force-pushed the feature/support-subjects branch 3 times, most recently from 5dc0831 to 5bcb557 Compare April 9, 2025 00:50
@abdulazimRabie
Copy link
Contributor Author

I think that we can write better code if we are having tow options

  • --subjects : for main categories [Math, Physics, ...]
  • --subSubjects: for sub categories [motion, gravity, ...]

If this is done, we can handle rootCategories by --subjects value , and handle cats by --subSubjects

@abdulazimRabie abdulazimRabie force-pushed the feature/support-subjects branch 2 times, most recently from 29fff86 to 830229c Compare April 9, 2025 03:33
Copy link
Contributor

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

I don't get how this code is supposed to work.

Can you please explain how to build (command to use) and provide a ZIM with only Biology exercises in English? I tried but it produced a ZIM with all exercises.

@abdulazimRabie abdulazimRabie force-pushed the feature/support-subjects branch 3 times, most recently from 6f1c681 to 11d151f Compare April 12, 2025 10:49
@abdulazimRabie
Copy link
Contributor Author

abdulazimRabie commented Apr 12, 2025

@benoit74 , I've made some changes. Now you can try phet2zim --includeLanguages 'en' --subjects 'biology'. Exported ZIM file must contain 7 exercises of biology in English.

Please, try also a combination of subjects :
phet2zim --includeLanguages 'en' --subjects 'biology, quantum, sound & waves'

@@ -13,6 +13,7 @@ import { iso6393To1 } from 'iso-639-3'
import { options, extractResources, loadLanguages, createFileContentProvider } from './utils.js'
import { rimraf } from 'rimraf'
import { fileURLToPath } from 'url'
import { categories } from 'steps/get/options.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

You should import options from export step, not get

README.md Outdated
Available only on GET step:
```bash
--withoutLanguageVariants ...
--subjects 'math,physics'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is misleading, text above says this is available only on GET step while --subjects MUST be passed to both GET and EXPORT steps

return formatcsv(langs)
}

export const formatSubjects = (subjects: string): string[] => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to slugify the subject? (not saying this is wrong, but it probably deserves a comment)

Copy link
Contributor Author

@abdulazimRabie abdulazimRabie Apr 14, 2025

Choose a reason for hiding this comment

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

I found that each time I deal with subjects (as array), I need to slugify all subjects again. So, i preferred to slugify all subjects from the beginning.

It also will reduce calling slugify to make code more readable.

Is it right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get it, sorry. Where are the subjects slugified in the first place? Why are they sluglified at this stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First , When I need to verify subjects (ensure that inputs are already existed in cats) , I use cats values (which are slugified). So, I need to slugify (input subjects). This happens in verifySubjects function.

Second, When I return only matched cats, I need to slugify the input subjects again

Over all , I try to avoid this : calling formatcsv (I prefer formatSubjects) and a lot of slugify(sub, {lower:true})

export const verifySubjects = (subjects: string): boolean => {
  return formatcsv(subjects).every((subj) => {
    return Object.values(cats).includes(slugify(subj, { lower: true }))
  })
}

export const getMatchedCats = (input: string) => {
  const formatedSubjects = formatcsv(input)
  const catsKeys = Object.keys(cats)
  const catsValues = Object.values(cats)
  const validCats = {}

  formatedSubjects.forEach((sub) => {
    const indexCat = catsValues.indexOf(slugify(sub, { lower: true }))
    if (indexCat >= 0) validCats[catsKeys[indexCat]] = catsValues[indexCat]
  })
  return validCats
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think there's no need for that. It's totally OK.

return formatcsv(subjects).map((subject) => slugify(subject, { lower: true }))
}

export const verifySubjects = (subjects: string): boolean => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why we lookup indexes to verify subject validity, wouldn't it be enough to check that all subjects are among categories?

@abdulazimRabie abdulazimRabie force-pushed the feature/support-subjects branch from 11d151f to 9092900 Compare April 14, 2025 10:42
@abdulazimRabie
Copy link
Contributor Author

@benoit74 , I've fixed requested changes.

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.

Support --subjects option in CLI
2 participants