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

Simplify dependencies handling for exercises in katas #1640

Closed
tcNickolas opened this issue Jun 13, 2024 · 6 comments · Fixed by #1674
Closed

Simplify dependencies handling for exercises in katas #1640

tcNickolas opened this issue Jun 13, 2024 · 6 comments · Fixed by #1674
Labels
enhancement New feature or request good first issue Good for newcomers katas

Comments

@tcNickolas
Copy link
Member

Is your feature request related to a problem? Please describe.
Currently each exercise in each kata has a field qsDependencies that lists files that need to be compiled together with the files of that specific exercise.

@[exercise]({
    "id": "marking_oracles__kth_bit",
    "title": "K-th Bit",
    "path": "./kth_bit/",
    "qsDependencies": [
        "../KatasLibrary.qs",
        "./Common.qs"
    ]
})

This is very repetitive, leads to hard-to-debug issues when contributing to the katas, and unnecessary, since the logic of including the files is the same across all katas.

Describe the solution you'd like
I'd like to drop this field and instead, when an exercise is created, to include the files using the following logic:

  1. Always include KatasLibrary.qs.
  2. If the kata to which this exercise belongs has a Common.qs file, include it.

Describe alternatives you've considered
We can keep the current approach.

@tcNickolas tcNickolas added enhancement New feature or request good first issue Good for newcomers katas labels Jun 13, 2024
@ggridin
Copy link
Contributor

ggridin commented Jun 24, 2024

@tcNickolas
I have few clarifying questions:

  1. Does it make sense to assume that KatasLibrary.qs is always 1 level above index.md (as of now)?
    If not, the logic could be adjusted to "If the kata to which this exercise belongs has ../KatasLibrary.qs (1 level up) file, include it".
  2. After this change, qsDependencies will be optional property, right?

May I work on this issue?

@tcNickolas
Copy link
Member Author

  1. Yes, looking at the current infra, including katas placed elsewhere into it would take significant changes, and we're not planning to do that. It's probably a good idea not to fail if the file doesn't exist, but we don't need to account for a possibility of it existing two levels up or in a completely different folder.
  2. We shouldn't need qsDependencies property at all! It was designed with just the first small set of katas as pilots, and now that we've added a lot more katas, we can see that we don't need to over-complicate their definitions, since they all follow the same structure easily.

Yes, absolutely!

@ggridin
Copy link
Contributor

ggridin commented Jun 24, 2024

Clarification for question 2
Am I correct that I should remove processing of qsDependencies property completely and remove all it's references from index.md's?

@tcNickolas
Copy link
Member Author

Yes please!

@ggridin
Copy link
Contributor

ggridin commented Jun 26, 2024

@tcNickolas
I have the fix. What upstream branch should I use to track the changes?
I do not have access to create remote branch.

@tcNickolas
Copy link
Member Author

You'll need to fork the microsoft/qsharp repository to your account, and then you will be able to create a branch in ggridin/qsharp repository and send the pull request from that branch to the main repo.

ggridin added a commit to ggridin/qsharp that referenced this issue Jun 27, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 28, 2024
…#1674)

Link to the issue: #1640

The following changes were done:
1. qsDependencies property is no longer processing
(generate_katas_content.js)
2. ../KatasLibrary.qs is included as implicit dependency
3. ./Common.qs is included as implicit dependency if present in current
kata
4. All qsDependencies properties are removed from index.md's
5. README.md is updated - reference to qsDependencies is removed

Testing done:
1. "Happy path": Windows cmd build: "python ./build.py --wasm --npm
--play" - success
   Local playground manual testing - no impact (Chrome and Edge)
2. No impact when qsDependencies are present in index.md
3. No impact when qsDependencies are present and empty in index.md

Question: should Ubuntu build be tested?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers katas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants