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

Unresolved feedback from #278 #306

Open
2 tasks
jnm opened this issue Nov 10, 2022 · 0 comments
Open
2 tasks

Unresolved feedback from #278 #306

jnm opened this issue Nov 10, 2022 · 0 comments

Comments

@jnm
Copy link
Member

jnm commented Nov 10, 2022

@noliveleger if there are more points that I've missed, please edit this description to add them 🙂

  • (1)

    Our coding habits have changed since my last review:
    We are now used to import from __future__ import annotations.
    def extend_survey(self, analysis_form: dict) -> None:
    and dict[str, str] should work too.
    Let's use the import then and change Dict to dict
    Originally posted by @noliveleger in MVP: Handle extra fields to export #278 (comment)

  • (2)

    @joshuaberetta what about this:

    item = re.sub(r'^_supplementalDetails/', '', item)
    # use explicit names for variable below.
    # `_` is dummy content, we don't use it and we don't care
    try:
        my_variable, *_, last_variable = item.split('/')
        *_, my_other_variable  = item.split('_')
    except ValueError:
        last_variable = None
    
    if last_variable and last_variable.startswith('transcript'):
        self.t_lang_codes.append(my_other_variable)
        _filter_fields.append(f"{my_variable}/transcript")
    else:
        _filter_fields.append(item)

    Originally posted by @noliveleger in MVP: Handle extra fields to export #278 (comment)

The if label is None concern is now tracked by #307

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

No branches or pull requests

1 participant