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

Feature/sckan-312 - add flag to neurondm ingestion script to disable the overwrite of KSs #317

Merged
merged 9 commits into from
Nov 12, 2024

Conversation

D-GopalKrishna
Copy link
Contributor

@D-GopalKrishna D-GopalKrishna commented Oct 11, 2024

Jira Link - SCKAN-312

Task description:

  1. add logic to disable overwrite of statements when the flag --disable_overwrite is used.
  2. Renamed overwritable_statements to overwritable_and_new_statements (I think it better represents the statements types)

More on the previous vs updated logic:

After the changes the definition of overwritable_and_new_statements changes. Whether the statements are overwritable or not depends on two utils - has_invalid_sentence and has_invalid_sentence.

(Previously and now) if the statement/sentence didn't exist we say it's not invalid. And therefore overwritable (new statement/sentence). This logic stays the same.

Previously - if the statement/sentence exists, and was overwritable (depending on if it is invalid/exported/compose_now) - then it is not invalid, and hence overwritable.
Now (after the change) - WE ONLY CHANGE THIS LOGIC above. This check only happens --disable_overwrite flag is not passed. If the flag is passed - we do not check this, and say the statement/sentence is invalid and hence - not overwritable.

Short demo video to showcase the same.

2024-10-11.14-26-48.mp4

demo after the logic is enabled.

2024-10-11.14-34-30.mp4

ddelpiano
ddelpiano previously approved these changes Oct 11, 2024
Copy link
Member

@ddelpiano ddelpiano left a comment

Choose a reason for hiding this comment

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

lgtm

@D-GopalKrishna D-GopalKrishna changed the base branch from feature/CSCKAN-311 to develop October 14, 2024 09:17
@D-GopalKrishna D-GopalKrishna dismissed ddelpiano’s stale review October 14, 2024 09:17

The base branch was changed.

afonsobspinto
afonsobspinto previously approved these changes Oct 14, 2024
Copy link
Member

@ddelpiano ddelpiano left a comment

Choose a reason for hiding this comment

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

@D-GopalKrishna could we add a test for this? thanks

@ddelpiano
Copy link
Member

@D-GopalKrishna both 311 and 312 are good for me, the only thing I would like to see are tests for both the features. Thanks a lot and great work :)

@D-GopalKrishna
Copy link
Contributor Author

NOTE: added tests. This increases the time to run the tests significantly - from literally under 1s to more than 400s.

afonsobspinto
afonsobspinto previously approved these changes Oct 31, 2024
@ddelpiano ddelpiano merged commit 54971a8 into develop Nov 12, 2024
1 check passed
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