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.

Sign up for GitHub

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

Refactor tasks to not fork, and always run in-process of the main execution pipeline #1613

Merged
merged 31 commits into from
Dec 13, 2022
+1,011 −787
Merged

Refactor tasks to not fork, and always run in-process of the main execution pipeline #1613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
24e157a
Check that Deno v1.23.x is used when building Taqueria
Dec 7, 2022
bc977f5
Temporary commit
Dec 7, 2022
6d4c9b0
Added means of registering internal tasks, and to process those tasks…
Dec 8, 2022
81c3a8f
Segmented global and internal tasks
Dec 8, 2022
2cc3bb8
Ensure that internal tasks are invoked
Dec 8, 2022
eddbde7
Fixed memory leak in test/e2e/utils
Dec 8, 2022
ebe670c
Fix broken test due to help output changing
Dec 8, 2022
ded3e1e
Fixed failing test spec due to help output changes
Dec 8, 2022
ca98a5c
Re-add afterAll hook to cleanup
Dec 8, 2022
ecd86da
Fix broken tests due to help output
Dec 8, 2022
e7b9aac
Fixed help output for contract types test spec
Dec 8, 2022
6412863
Committing work thus far
Dec 9, 2022
60b72ed
Accomodate aliases when determining whether a task is runninng
Dec 9, 2022
4dfd48f
Fixed problem with taq not outputting in non-taquified directory
Dec 9, 2022
1736d8d
Merged main into refactor-internal-tasks
Dec 9, 2022
38e3a76
Skip pinata help output tests
Dec 9, 2022
897629a
Skipping some tests due to help output changing
Dec 9, 2022
7f15da8
Assure that we can identify whether tasks are being run which have sp…
Dec 9, 2022
da066d3
Fixed issue with parsedArgs._ being modified after parseArgs is run
Dec 9, 2022
f704bbf
Ensure that detection for whether a given task is running is working
Dec 10, 2022
c1d4a33
Merge branch 'main' into refactor-internal-tasks
Dec 10, 2022
aca2f32
Skip some tests that fail due to change in help output
Dec 10, 2022
d36d379
Add missing aliases to some registered tasks
Dec 10, 2022
4c10da2
Skipped more tests due to changes in help text output
Dec 10, 2022
faf30f1
Fixed issues with failing tests in metadata test suite due to legacy …
Dec 10, 2022
79f3556
Fixed broken ligo test due to skipping a previous test
Dec 10, 2022
19f6a91
Removed debugger statements
Dec 10, 2022
94408ce
Fixed typo
Dec 10, 2022
10caa14
Added link to issue for todo item
Dec 10, 2022
752ea15
Addressed PR comments
Dec 12, 2022
b405eb0
Merge branch 'prerelease-0.25.0' into refactor-internal-tasks
hu3man Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions analytics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,15 @@ export const inject = (deps: UsageAnalyticsDeps) => {
return taqResolve('');
}
}),
mapRej(() =>
option === OPT_IN
? 'The command "taq opt-in" is ignored as this might be the first time running Taqueria...'
: 'The command "taq opt-out" is ignored as this might be the first time running Taqueria...'
mapRej(previous =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need it? I thought this would not be needed after the internal task refactor

Copy link
Collaborator 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 be honest. If not, I assume we'll address that in a different ticket/PR.
The acceptance criteria for this particular issue/PR was:

Refactor remaining internal tasks so that the execution flow matches that of plugin-provided tasks

As that criteria is a little vague, I interpreted it to be:

Refactor tasks to not fork but run in the main execution pipeline

The motive for that , as far as I was aware, was to resolve a) memory leaks; b) concurrency issues; c) make the code easier to reason about; d) ability to ask for --help for built-in tasks

I had thought that the only thing holding back GA tracking was issue B above.

--
I modified the following code strictly for type correctness, as this section of code was returning Future<string, unknown> instead of Future<TaqError, unknown>:

Before:

mapRej(() =>
  option === OPT_IN
	  ? 'The command "taq opt-in" is ignored as this might be the first time running Taqueria...'
	  : 'The command "taq opt-out" is ignored as this might be the first time running Taqueria...'
)

After:

mapRej(previous => TaqError.create({
	kind: 'E_OPT_IN_WARNING',
	msg: option === OPT_IN
		? 'The command "taq opt-in" is ignored as this might be the first time running Taqueria...'
		: 'The command "taq opt-out" is ignored as this might be the first time running Taqueria...',
	previous,
	context: option,
})

If the above code isn't needed anymore, great - but I'd probably prefer making that adjustment in a different PR, as I cannot recall the reason why Jimmy added this code, and too, I think that's outside of the scope I had in mind in this PR. Would you be fine if we created an issue for this instead, Ali? I would do so, but given that you recognized this, I assume you might have more context/recollection about why this code was here to start with.

TaqError.create({
kind: 'E_OPT_IN_WARNING',
msg: option === OPT_IN
? 'The command "taq opt-in" is ignored as this might be the first time running Taqueria...'
: 'The command "taq opt-out" is ignored as this might be the first time running Taqueria...',
previous,
context: option,
})
),
);

Expand Down
Loading