-
Notifications
You must be signed in to change notification settings - Fork 1
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
KMS-532: Create CSV output for schemes #17
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17 +/- ##
==========================================
+ Coverage 99.36% 99.59% +0.23%
==========================================
Files 42 62 +20
Lines 470 742 +272
Branches 108 159 +51
==========================================
+ Hits 467 739 +272
Misses 3 3 ☔ View full report in Codecov by Sentry. |
* @param {string} scheme - The scheme to check. | ||
* @returns {boolean} True if the scheme is a CSV provider URL flag, false otherwise. | ||
*/ | ||
const getCsvProviderUrlFlag = (scheme) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to isCsvProviderUrlFlag.js to denote it returns a boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -33,6 +34,11 @@ import { toSkosJson } from '@/shared/toSkosJson' | |||
*/ | |||
export const getConcepts = async (event) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is up with the tests, says this line through 134 are no longer covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might have issue with local branch KMS-532
vi.mock('../isCsvProviderUrlFlag') | ||
|
||
describe('buildHierarchicalCsvPaths', () => { | ||
test('should traverse the graph and return paths', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have describe('when.....) test('should....) structure for all our tests for consistency with MMT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
vi.restoreAllMocks() | ||
}) | ||
|
||
it('should create a CSV string with metadata and headers', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'test' not 'it'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
* @returns {Promise<String>} A promise that resolves with the CSV string output | ||
*/ | ||
// eslint-disable-next-line max-len | ||
export const createCsv = async (csvMetadata, csvHeaders, values) => new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* } | ||
*/ | ||
|
||
export const createCsvForScheme = async (scheme) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* @param {string} scheme - The scheme name for the XML representation URL | ||
* @returns {string[]} An array of metadata strings | ||
*/ | ||
export const getCsvMetadata = async (scheme) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Overall PR looks really organized with lots of great comments. Only overarching comment is that some of the functions could probably use examples to give an idea of what the expected data looks like (difficult to tell since it isn't a typed language). You should be able to use continue to help with this. Other than that, anytime we have a list of parameters more than 2 or 3 we should use a dictionary instead, it makes it easier to read. |
* | ||
* // Example output: | ||
* // { | ||
* // 'http://example.com/provider1': ['BusinessObject1', 'BusinessObject2'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a map of provider ids to their corresponding urls, if so maybe just say 'URL1', 'URL2'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Overview
What is the feature?
Create CSV output for given scheme
What is the Solution?
CSV output interface added
What areas of the application does this impact?
CSV output interface
Testing
For local dev env, use, example for platforms:
http://localhost:4001/dev/concepts?format=csv&scheme=platforms
Attachments
N/A
Checklist