Skip to content

Conversation

@laurentiumagureanu
Copy link
Contributor

Description

Added copyright headers to CSS and graphql files

Related Issue

CIF-1996

Motivation and Context

According to a comment on a completely different issue on OSS Advisory Board we should have copyright headers on CSS and GraphQL files as well.

How Has This Been Tested?

Unit tests
Manually

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #518 (c53c9f3) into master (564f8fc) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #518   +/-   ##
=========================================
  Coverage     87.59%   87.59%           
  Complexity     1232     1232           
=========================================
  Files           232      232           
  Lines          5875     5875           
  Branches        857      857           
=========================================
  Hits           5146     5146           
  Misses          574      574           
  Partials        155      155           
Flag Coverage Δ Complexity Δ
integration 65.22% <ø> (ø) 0.00 <ø> (ø)
jest 83.33% <ø> (ø) 0.00 <ø> (ø)
karma 94.54% <ø> (ø) 0.00 <ø> (ø)
unittests 87.93% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 564f8fc...c53c9f3. Read the comment docs.

@dplaton
Copy link
Contributor

dplaton commented Mar 17, 2021

I remember we had some tooling that collected the queries made by the React components (@herzog31 knows more). I think this tooling wouldn't work with the change you did to the .graphql files.

Copy link
Contributor

@cjelger cjelger left a comment

Choose a reason for hiding this comment

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

Related to @dplaton's comment, this will break https://github.com/adobe/commerce-cif-graphql-integration-reference/blob/master/src/documentation/generate.js#L69-L73 but it would also be simple to fix it there.

let schema = buildClientSchema(magentoSchema.data);

it.each(files)('validates the GraphQL request from %s', file => {
let query = fs.readFileSync(path.join(__dirname, '..', file), 'UTF-8'); // eslint-disable-line
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 rather simply dynamically import each query/file here as any normal exported js module value.

Copy link
Contributor Author

@laurentiumagureanu laurentiumagureanu Mar 17, 2021

Choose a reason for hiding this comment

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

tried that. didn't work for some reason.
Got some can't access length property of undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there seems to be a difference between the parsed query and the imported one:
parsed query:

    {
        kind: 'Document',
        definitions: [
          {
            kind: 'OperationDefinition',
            operation: 'query',
            name: [Object],
            variableDefinitions: [],
            directives: [],
            selectionSet: [Object],
            loc: [Object]
          }
        ],
        loc: { start: 0, end: 146 }
     } 

imported query :

    {
        default: {
          kind: 'Document',
          definitions: [ [Object] ],
          loc: { start: 0, end: 147 }
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore my comments. I'll update the code.

@laurentiumagureanu
Copy link
Contributor Author

@dplaton, @cjelger I opened a PR for the tool you mentioned
adobe/commerce-cif-graphql-integration-reference#40

@herzog31
Copy link
Member

Did anyone ask for an exception? Changing all the graphql files including the webpack build and tests, just for the sake of adding copyright headers is ridiculous. Can't we simply put a license file in the folder where all queries are and say that it applies to all?

I'm fine with the changes to the css files.

@LSantha LSantha merged commit 410852a into master Mar 18, 2021
@LSantha LSantha deleted the tasks/CIF-1996 branch March 18, 2021 16:17
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.

6 participants