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

Codemeta v3.0 - import #40

Merged
merged 8 commits into from
May 6, 2024

Conversation

hjonin
Copy link
Contributor

@hjonin hjonin commented Apr 5, 2024

Allow codemeta.json v3.0 file import.

Follows #34.

@hjonin hjonin mentioned this pull request Apr 5, 2024
Copy link
Member

@progval progval left a comment

Choose a reason for hiding this comment

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

Some nitpicks in the test names before I noticed this PR includes some code from #34 so I'm going to wait for #34 to be merged before I keep reviewing this

cypress/integration/basics.js Outdated Show resolved Hide resolved
cypress/integration/basics.js Outdated Show resolved Hide resolved
cypress/integration/basics.js Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@hjonin hjonin force-pushed the feat/gh-23-codemeta-v3-import branch from 7943901 to a87777f Compare April 30, 2024 08:41
@hjonin hjonin changed the title Codemeta v3.0 - import [WIP] Codemeta v3.0 - import Apr 30, 2024
@hjonin hjonin changed the title [WIP] Codemeta v3.0 - import Codemeta v3.0 - import Apr 30, 2024
@hjonin
Copy link
Contributor Author

hjonin commented Apr 30, 2024

This should be ready for review :)

cypress/integration/basics.js Outdated Show resolved Hide resolved
cypress/integration/basics.js Outdated Show resolved Hide resolved
cypress/integration/persons.js Show resolved Hide resolved
Comment on lines +649 to +656
cy.get('#author_nb').should('have.value', '1');
cy.get('#author_1_givenName').should('have.value', 'Jane');
cy.get('#author_1_roleName_0').should('have.value', 'Maintainer');
cy.get('#author_1_startDate_0').should('have.value', '2024-04-04');
cy.get('#author_1_endDate_0').should('have.value', '2024-05-05');
cy.get('#author_1_roleName_1').should('have.value', 'Developer');
cy.get('#author_1_startDate_1').should('have.value', '2024-03-04');
cy.get('#author_1_endDate_1').should('have.value', '2024-04-03');
Copy link
Member

Choose a reason for hiding this comment

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

neat

cypress/integration/persons.js Outdated Show resolved Hide resolved
@@ -683,4 +758,93 @@ describe('Multiple authors', function () {
]
});
});

it('who both have roles can be imported', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('who both have roles can be imported', function () {
it('can be imported', function () {

ditto (or probably move it to a new describe('One author with a role', function () { block)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure to understand about this one?

Copy link
Member

Choose a reason for hiding this comment

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

ah nevermind, it makes sense to me now

Comment on lines +324 to +327
// TODO should test more properties for equality?
return author1.givenName === author2.givenName
&& author1.familyName === author2.familyName
&& author1.email === author2.email;
Copy link
Member

Choose a reason for hiding this comment

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

The id is what matters in JSON-LD. Without it, JSON-LD says we should consider them different nodes. (see also #42, which doesn't need to be fixed before merging this PR)

However, given that these are the only ways we display an author, I think it's fine to have these three equalities as a fallback when both nodes are missing an id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'm looking into #42 !

js/codemeta_generation.js Show resolved Hide resolved
js/validation/index.js Outdated Show resolved Hide resolved
@hjonin hjonin requested a review from progval May 6, 2024 15:29
@progval
Copy link
Member

progval commented May 6, 2024

thanks!

@progval progval merged commit 9bf79ad into codemeta:master May 6, 2024
1 check passed
@progval
Copy link
Member

progval commented May 6, 2024

And that's the 100th commit in this repository :)

@hjonin
Copy link
Contributor Author

hjonin commented May 6, 2024

Thank you for your reviews and comments!

@hjonin hjonin deleted the feat/gh-23-codemeta-v3-import branch May 7, 2024 08:15
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.

2 participants