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

Define an API to query records history #875

Merged
merged 74 commits into from
Jun 15, 2022
Merged

Define an API to query records history #875

merged 74 commits into from
Jun 15, 2022

Conversation

Ndpnt
Copy link
Member

@Ndpnt Ndpnt commented Jun 8, 2022

  • Define and use an API to query records history through GitRepository and MongoRespository
  • Add a migration script as an example of use
  • Refactor storage-adapters to repositories to properly the Repository adn Data Mapper patterns
  • Significantly improve performance of git-related queries

Ndpnt added 30 commits May 25, 2022 17:33
GitHub displays the committer date so if we only use `--date` option to
git commit, the date displayed on GitHub UI when browsing history is confusing
No more git checkout drastically improves performance and ensures that the
repository is not checked out to a commit in the middle of the history
in case of an error.
Also allow to order commit before iteration.
Also unify and improve returned commits data
Lazy load record content
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

First pass done.

MattiSG
MattiSG previously approved these changes Jun 13, 2022
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@martinratinaud martinratinaud left a comment

Choose a reason for hiding this comment

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

Great job again

@@ -106,3 +106,10 @@ overrides:
rules:
func-names: 0
import/no-extraneous-dependencies: 0
- files:
- src/**/*[iI]nterface.js
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need both I and i ?

I see only one interface.js file in lower case

Copy link
Member Author

@Ndpnt Ndpnt Jun 15, 2022

Choose a reason for hiding this comment

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

At the beginning my file was named repositoryInterface.js and I renamed it after.
The rules I defined will be true for all interfaces so I support all naming.

@@ -0,0 +1,61 @@
import path from 'path';
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure about when this script should be used and if it can be an safely on any repository.

A comment at the top of this file would be enough if it states

  • purpose
  • sample usage
  • ran twice will not produce error

@@ -0,0 +1,23 @@
import fsApi from 'fs';
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be only working for git so 2 options

  • move it to git folder
  • name it importReadmeInGit

@@ -41,9 +41,9 @@ export default class Archivist extends events.EventEmitter {
return Object.keys(this.services);
}

constructor({ storage: { versions, snapshots } }) {
constructor(config) {
Copy link
Member

Choose a reason for hiding this comment

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

Passing the whole config, while being convenient, makes it very difficult to know what is used.

I suggest only passing recorder config like

constructor({ recorder }) {

and call it with

app = new Archivist({ recorder: config.get("recorder")});

src/archivist/recorder/repositories/git/dataMapper.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
src/archivist/recorder/index.test.js Show resolved Hide resolved
src/archivist/recorder/index.test.js Show resolved Hide resolved
@@ -0,0 +1,212 @@
import fsApi from 'fs';
Copy link
Member

Choose a reason for hiding this comment

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

This implementation seems very clear to me but have the downside that every migration created will have commitable changes.

I think this may not be part of this PR but it would be great to have the config loaded from a gitignored file .migration.json
Only change needed would be to add the type of repository to it so that the right repository class is used

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'm going to do this in another PR to allow merging this one.

@MattiSG MattiSG dismissed their stale review June 15, 2022 12:31

There were many changes after my review

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

I'm impressed that we can drop src/index.js altogether. With these incremental reviews, I can't really be sure how appropriate it is.

src/archivist/index.js Outdated Show resolved Hide resolved
src/archivist/recorder/index.test.js Show resolved Hide resolved
@Ndpnt Ndpnt merged commit 81e0400 into main Jun 15, 2022
@Ndpnt Ndpnt deleted the migrate-services branch June 15, 2022 15:05
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.

3 participants