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

feat: Rename from sugar to core module name #101

Merged
merged 3 commits into from
Jan 15, 2024
Merged

feat: Rename from sugar to core module name #101

merged 3 commits into from
Jan 15, 2024

Conversation

esloch
Copy link
Contributor

@esloch esloch commented Dec 20, 2023 •
edited
Loading

Pull Request description

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 requested changes Dec 20, 2023
View reviewed changes
src/sugar/cli.py Outdated
@@ -6,7 +6,7 @@
from pathlib import Path
from typing import Tuple

from sugar import Sugar
from core import Sugar
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from core import Sugar
from sugar.core import Sugar

src/sugar/core.py Outdated
@@ -13,7 +13,7 @@

from jinja2 import Template

from sugar import __version__
from core import __version__
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from core import __version__
from sugar import __version__

tests/test_containers_sugar.py Outdated
@@ -4,7 +4,7 @@

import pytest

from sugar import Sugar
from core import Sugar
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from core import Sugar
from sugar.core import Sugar

@xmnlab
Copy link
Member

xmnlab commented Dec 21, 2023

@esloch I just merged this PR: #102
could you rebase your branch on top of the upstream main?

@esloch
Copy link
Contributor Author

esloch commented Dec 21, 2023

@esloch I just merged this PR: #102 could you rebase your branch on top of the upstream main?

I think that we need to update the poetry.lock.

@xmnlab
Copy link
Member

xmnlab commented Dec 21, 2023

Could you remove and recreate it please?

@esloch
Copy link
Contributor Author

esloch commented Dec 22, 2023

Could you remove and recreate it please?

I removed the "poetry.lock" and then run the "poetry update":

(sugar) ✔ /workspace/repos/osl/sugar [rename-module|● 1] 
21:39 $ poetry update
Updating dependencies
Resolving dependencies... (65.4s)

The current project's Python requirement (>=3.8.1) is not compatible with some of the required packages Python requirement:
  - makim requires Python >=3.8.1,<4.0.0, so it will not be satisfied for Python >=4.0.0

Because containers-sugar depends on makim (1.8.3) which requires Python >=3.8.1,<4.0.0, version solving failed.

  • Check your dependencies Python requirement: The Python requirement can be specified via the `python` or `markers` properties
    
    For makim, a possible solution would be to set the `python` property to ">=3.8.1,<4.0.0"

@xmnlab
Copy link
Member

xmnlab commented Dec 22, 2023

@esloch the issue with the installation was fixed (I hope!)
now the error is about the tests.

@esloch
Copy link
Contributor Author

esloch commented Dec 22, 2023

@esloch the issue with the installation was fixed (I hope!) now the error is about the tests.

Thanks Ivan!

@xmnlab
Copy link
Member

xmnlab commented Dec 24, 2023

@esloch , just cut a new release. please, rebase your branch on top of the upstream main

@xmnlab xmnlab marked this pull request as ready for review January 15, 2024 12:02
xmnlab
xmnlab approved these changes Jan 15, 2024
View reviewed changes
Copy link
Member

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @esloch

@xmnlab xmnlab merged commit 40dace1 into main Jan 15, 2024
8 checks passed
@xmnlab xmnlab deleted the rename-module branch January 15, 2024 12:03
joel5vega pushed a commit to joel5vega/containers-sugar that referenced this pull request Jan 19, 2024
@esloch @joel5vega
feat: Rename from sugar to core module name (osl-incubator#101)
f5597e7
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 1.10.0 🎉

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

@xmnlab xmnlab xmnlab approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@esloch @xmnlab