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.

Sign up for GitHub

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

Jump to bottom

fix: remove check for header by sniffer #227

Merged
merged 2 commits into from
Feb 23, 2023
Merged

fix: remove check for header by sniffer #227

merged 2 commits into from
Feb 23, 2023

Conversation

Midnighter
Copy link
Contributor

The csv.Sniffer's has_header method produces false negatives. Since we anyway check for the presence of columns, we can simply remove this check.

Fix #226

@Midnighter
fix: remove check for header by sniffer
26dc2e4 
The `csv.Sniffer`'s `has_header` method produces false negatives. Since
we anyway check for the presence of columns, we can simply remove this
check.
@github-actions
Copy link

github-actions bot commented Feb 23, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 1be550c

+| ✅ 157 tests passed       |+
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in WorkflowMain.groovy: Add Zenodo DOI for pipeline after first release
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline

✅ Tests passed:

Run details

  • nf-core/tools version 2.7.2
  • Run at 2023-02-23 15:58:42

@jfy133
Copy link
Member

jfy133 commented Feb 23, 2023

CHANGE LOG please (now we are in production phase xD). But otherwise LGTM if we already are checking for those columns by name

@Midnighter
Copy link
Contributor Author

We are

required_columns = {"sample", "fasta"}
    # See https://docs.python.org/3.9/library/csv.html#id3 to read up on `newline=""`.
    with file_in.open(newline="") as in_handle:
        reader = csv.DictReader(in_handle, dialect=sniff_format(in_handle))
        # Validate the existence of the expected header columns.
        if not required_columns.issubset(reader.fieldnames):
            req_cols = ", ".join(required_columns)
            logger.critical(f"The sample sheet **must** contain these column headers: {req_cols}.")
            sys.exit(1)

@jfy133 jfy133 self-requested a review February 23, 2023 16:05
@Midnighter Midnighter merged commit cc95681 into dev Feb 23, 2023
@Midnighter Midnighter deleted the fix-sheet-check branch February 23, 2023 22:00
@jfy133
Copy link
Member

jfy133 commented Feb 24, 2023

@Midnighter will you do a PR into the ipeline template too?

jfy133 added a commit to nf-core/tools that referenced this pull request Feb 24, 2023
@jfy133
Remove problematic sniffer code resulting in no header found even whe…
6c0bcf5 
…n it exists

See nf-core/funcscan#227
@jfy133 jfy133 mentioned this pull request Feb 24, 2023
Merged
4 tasks
@jasmezz jasmezz mentioned this pull request Feb 27, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfy133 jfy133 jfy133 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overly long string size in input samplesheet causes samplesheet check to erroenously report missing header
2 participants
@Midnighter @jfy133