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

WIP | Refactor existing parser using typescript #164

Open
wants to merge 7 commits into
base: vue3
Choose a base branch
from

Conversation

Efode-r2d2
Copy link
Collaborator

No description provided.

@Efode-r2d2
Copy link
Collaborator Author

@Midnighter this is work in progress(needs a lot of work and I am working on it).

Copy link
Collaborator

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

The work seems in progress indeed and only small changes to the original so far. You have chosen to start with the perhaps the most complicated part of the application. Maybe we should pair up on changing it?

@@ -8,6 +8,7 @@ services:
- "127.0.0.1:${UI_PORT:-8000}:8000"
volumes:
- "./:/app"
- "/app/node_modules"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have my doubts about this change. Could it be that you didn't rebuild the image after changing dependencies?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using a long array and define everything in script, could we create pairs of files (markdown, JSON) perhaps, that we load in the tests? Seems easier to read and maintain to me.

@@ -16,6 +16,7 @@
"format": "prettier --write src/"
},
"dependencies": {
"antlr4": "^4.13.1-patch-1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you have a look at the TypeScript enabled antlr4 packages, are they any good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would change this then depending on what we do with the constants.ts.

@@ -0,0 +1,347 @@
import { cloneDeep } from "lodash";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typically, there is no more need for lodash when targetting ES6. We should try to get rid of this dependency.

return printer.adr;
}

export function adr2md(adrToParse) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a huge fan of constructing the strings this way. Perhaps we can use template strings and some kind of fluid interface to construct markdown.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or a markdown builder service where we add sections...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would consider ADR a domain model, but perhaps the parser rather works with a data transfer object, not sure yet.

Comment on lines 17 to 53
this.title = title || "";
this.status = status || "";
this.deciders = deciders || "";
this.date = date || "";
this.technicalStory = technicalStory || "";
this.contextAndProblemStatement = contextAndProblemStatement || "";
this.decisionDrivers = decisionDrivers || [];
this.highestOptionId = 0;
this.consideredOptions = [];
if (consideredOptions && consideredOptions.length > 0) {
for (let i = 0; i < consideredOptions.length; i++) {
this.addOption(consideredOptions[i]);
}
}
this.decisionOutcome = decisionOutcome || {
chosenOption: "",
explanation: "",
positiveConsequences: [],
negativeConsequences: []
};
this.links = links || [];

// Assure invariants for decisionOutcome attribute
if (!Object.prototype.hasOwnProperty.call(this.decisionOutcome, "chosenOption")) {
this.decisionOutcome.decisionOutcome = "";
}
if (!Object.prototype.hasOwnProperty.call(this.decisionOutcome, "explanation")) {
this.decisionOutcome.explanation = "";
}
if (!Object.prototype.hasOwnProperty.call(this.decisionOutcome, "positiveConsequences")) {
this.decisionOutcome.positiveConsequences = [];
}
if (!Object.prototype.hasOwnProperty.call(this.decisionOutcome, "negativeConsequences")) {
this.decisionOutcome.negativeConsequences = [];
}

this.cleanUp();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Constructors should be extremely simple. Rather have a separate factory method with more logic.

@Efode-r2d2
Copy link
Collaborator Author

The work seems in progress indeed and only small changes to the original so far. You have chosen to start with the perhaps the most complicated part of the application. Maybe we should pair up on changing it?

Yeah, would love to pair up.

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.

None yet

2 participants