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

Jump to bottom

fix: Fix profile's mechanism and update OSL profile #189

Merged
merged 8 commits into from
Dec 8, 2023
Merged

fix: Fix profile's mechanism and update OSL profile #189

merged 8 commits into from
Dec 8, 2023

Conversation

xmnlab
Copy link
Member

@xmnlab xmnlab commented Dec 3, 2023 •
edited
Loading

Pull Request description

This PR aims to:

  • improve the UI experience
  • remove some code about depends_on that was not fully implemented
  • fix some templates files
  • replace some linter tools by ruff/ruff format
  • fix the git workflow

How to test these changes

  • ...

Pull Request checklists

This PR is a:

  • bug-fix
  • new feature
  • maintenance

About this PR:

  • it includes tests.
  • the tests are executed on CI.
  • the tests generate log file(s) (path).
  • pre-commit hooks were executed locally.
  • this PR requires a project documentation update.

Author's checklist:

  • I have reviewed the changes and it contains no misspelling.
  • The code is well commented, especially in the parts that contain more complexity.
  • New and old tests passed locally.

Additional information

Reviewer's checklist

Copy and paste this template for your review's note:

## Reviewer's Checklist

- [ ] I managed to reproduce the problem locally from the `main` branch
- [ ] I managed to test the new changes locally
- [ ] I confirm that the issues mentioned were fixed/resolved

@xmnlab xmnlab changed the title WIP fix: Fix profile's mechanism and update OSL profile fix: Fix profile's mechanism and update OSL profile Dec 3, 2023
Saransh-cpp
Saransh-cpp reviewed Dec 5, 2023
View reviewed changes
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @xmnlab! Looks very good to me. I'll try using this branch locally this week to test if everything is working right!

pyproject.toml
]

[tool.poetry.scripts]
"scicookie" = "scicookie.__main__:app"

[tool.poetry.dependencies]
python = ">=3.8.1,<4"
python = ">=3.8.1,<3.12"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we're not supporting Python 3.12?

Copy link
Member Author

Choose a reason for hiding this comment

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

not exactly, but we would need to test it first on CI
but the other deps we have like makim and sugar, for example, is not working with 3.12
we can add support for that in a follow-up
but I think it is better to add support for python version incrementally instead of <4, because it could silently break

pyproject.toml
@@ -85,11 +72,18 @@ select = [
"RUF", # Ruff-specific rules
Copy link
Member

Choose a reason for hiding this comment

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

The select section should now be under [tool.ruff.lint]

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, I am going to change that in a bit. thanks for the update about that!

pyproject.toml Outdated
@@ -85,11 +72,18 @@ select = [
"RUF", # Ruff-specific rules
"I001", # isort
]
fixable = ["I001"]
fix = true
Copy link
Member

Choose a reason for hiding this comment

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

I think this should not be here; instead, we should have --fix in args in `.pre-commit-config.yaml

Suggested change
fix = true

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 will going to do that in a bit, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems we don't need that for ruff format :)
so it would be totally fine to just remove that from pyproject.
thanks for caching that!

src/scicookie/ui.py
from jinja2 import Template

from scicookie.logs import SciCookieErrorType, SciCookieLogs

# Initialize Colorama
init()


def _create_question(
question_id: str, question: dict
) -> Optional[inquirer.questions.Question]:
Copy link
Member

Choose a reason for hiding this comment

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

The types in the codebase can be simplified, for example -

Suggested change
) -> Optional[inquirer.questions.Question]:
) -> inquirer.questions.Question | None:

But thus is not related to this PR. I can create a follow-up PR to update the typing hints.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice! it would be great to have a follow-up pr for that :) thank you

@xmnlab
Copy link
Member Author

xmnlab commented Dec 7, 2023

thank you so much @Saransh-cpp and @Anavelyz for the review and feedback.

@xmnlab
Copy link
Member Author

xmnlab commented Dec 7, 2023

@Saransh-cpp is it ready to go? or are you still checking that locally?

@xmnlab
Copy link
Member Author

xmnlab commented Dec 8, 2023

I am merging this PR. but feel free to point me any other issue and I can work on a follow-up.
thanks again @Saransh-cpp and @Anavelyz

@xmnlab xmnlab merged commit 4eaf7b4 into osl-incubator:main Dec 8, 2023
11 checks passed
@xmnlab xmnlab deleted the fix-profiles branch December 8, 2023 14:27
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 8, 2023

🎉 This PR is included in version 0.6.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Saransh-cpp Saransh-cpp Saransh-cpp left review comments

@Anavelyz Anavelyz Awaiting requested review from Anavelyz

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xmnlab @Saransh-cpp