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

Dev container clean up #78

Merged
merged 28 commits into from
Mar 6, 2024
Merged

Dev container clean up #78

merged 28 commits into from
Mar 6, 2024

Conversation

SKairinos
Copy link
Contributor

@SKairinos SKairinos commented Mar 5, 2024

This change is Reviewable

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

Reviewed 25 of 25 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @SKairinos)


.submodules/__main__.py line 13 at r1 (raw file):

present in the global-config, they will remain. However, in some cases, the
behavior is to override the values (values not present in the global-config will
be removed).  

Extra whitespaces


.submodules/__main__.py line 15 at r1 (raw file):

be removed).  
"""

Remove line


.submodules/__main__.py line 49 at r1 (raw file):

class VSCode:
    """JSON files contained within the .vscode directory."""

Remove line


.submodules/__main__.py line 63 at r1 (raw file):

class SubmoduleConfig:
    """A configuration for a submodule."""

Remove line


.submodules/__main__.py line 96 at r1 (raw file):

    )

    return json.loads(raw_json_without_comments)

Do we need to handle the case where this "causes Python to explode" ie, if it tries to json.loads a jsonc file which has any of the "unauthorised" comments? I guess it would already throw an error anyway?


.submodules/__main__.py line 114 at r1 (raw file):

def merge_json_lists(current: JsonValue, latest: JsonList):

after our discussion in the meeting I wonder what we should settle on for these, terminology-wise.
I'm rarely a big fan of "current" and "latest" because I find they can get confusing pretty easily.

You mentioned "global" and "local" which I find a bit better, but could still be confusing, maybe?
What about "parent" and "child", or something like that, which clearly identifies which one is being inherited from and which one is extending?

Happy to discuss more.


.submodules/__main__.py line 390 at r1 (raw file):

        for inheritance in config_inheritances[::-1]:
            inheritances.insert(index, inheritance)

What is the reason for inserting the inheritances in inheritances in the reverse order that they are in config_inheritances?


.submodules/__main__.py line 411 at r1 (raw file):

    # Process each config.
    for key, config in configs.items():
        # Skip config if it's not going to merged into any submodules.

to be merged


.vscode/settings.json line 17 at r1 (raw file):

  //   "-m",
  //   "black"
  // ],

Remove if not needed

Code quote:

  // "black-formatter.args": [
  //   "--config",
  //   "pyproject.toml"
  // ],
  // "black-formatter.path": [
  //   ".venv/bin/python",
  //   "-m",
  //   "black"
  // ],

.vscode/settings.json line 68 at r1 (raw file):

  //   "-c=pyproject.toml",
  //   "."
  // ],

Remove if not needed

Code quote:

  // "isort.args": [
  //   "--settings-file=pyproject.toml"
  // ],
  // "isort.path": [
  //   ".venv/bin/python",
  //   "-m",
  //   "isort"
  // ],
  // "mypy-type-checker.args": [
  //   "--config-file=pyproject.toml"
  // ],
  // "mypy-type-checker.path": [
  //   ".venv/bin/python",
  //   "-m",
  //   "mypy"
  // ],
  // "pylint.args": [
  //   "--rcfile=pyproject.toml"
  // ],
  // "pylint.path": [
  //   ".venv/bin/python",
  //   "-m",
  //   "pylint"
  // ],
  // "python.defaultInterpreterPath": ".venv/bin/python",
  // "python.testing.pytestArgs": [
  //   "-n=auto",
  //   "-c=pyproject.toml",
  //   "."
  // ],

Copy link
Contributor Author

@SKairinos SKairinos left a comment

Choose a reason for hiding this comment

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

Reviewable status: 23 of 30 files reviewed, 10 unresolved discussions (waiting on @faucomte97)


.submodules/__main__.py line 13 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Extra whitespaces

Done.


.submodules/__main__.py line 15 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove line

can't. black requires this formatting.


.submodules/__main__.py line 49 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove line

can't. black requires this formatting.


.submodules/__main__.py line 63 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove line

can't. black requires this formatting.


.submodules/__main__.py line 96 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Do we need to handle the case where this "causes Python to explode" ie, if it tries to json.loads a jsonc file which has any of the "unauthorised" comments? I guess it would already throw an error anyway?

Yes, exactly. No need for us to do anything. If there are comments still in the doc, json.loads will fail to parse the json and raise a parse error.


.submodules/__main__.py line 114 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

after our discussion in the meeting I wonder what we should settle on for these, terminology-wise.
I'm rarely a big fan of "current" and "latest" because I find they can get confusing pretty easily.

You mentioned "global" and "local" which I find a bit better, but could still be confusing, maybe?
What about "parent" and "child", or something like that, which clearly identifies which one is being inherited from and which one is extending?

Happy to discuss more.

change to be "global" for workspace-level-submodule-config and "submodule" for current-submodule-config.


.submodules/__main__.py line 390 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

What is the reason for inserting the inheritances in inheritances in the reverse order that they are in config_inheritances?

The reason we need to insert the inheritance in reverse order is because we are inserting all the inheritances in the same index.

For example, say:

config_inheritances = [b, c, d]
index = 1
inheritances = [a, e]

then we would do:
insert d in at index 1: [a, d, e]
insert c in at index 1: [a, c, d, e]
insert b in at index 1: [a, b, c, d, e]

final result is the correct order of inheritances: [a, b, c, d, e]


.submodules/__main__.py line 411 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

to be merged

Done.


.vscode/settings.json line 17 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove if not needed

Done.


.vscode/settings.json line 68 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove if not needed

Done.

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)


.submodules/helpers.py line 48 at r2 (raw file):

def merge_json_lists(current: "JsonValue", latest: "JsonList"):

still has current and latest


.submodules/helpers.py line 66 at r2 (raw file):

def merge_json_dicts(current: "JsonValue", latest: "JsonDict"):

still has current and latest


.submodules/helpers.py line 101 at r2 (raw file):

def merge_json_lists_of_json_objects(

still has current and latest

Copy link
Contributor Author

@SKairinos SKairinos left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faucomte97)


.submodules/helpers.py line 48 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

still has current and latest

Done.


.submodules/helpers.py line 66 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

still has current and latest

Done.


.submodules/helpers.py line 101 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

still has current and latest

Done.

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)

@faucomte97 faucomte97 merged commit 8f647fe into main Mar 6, 2024
2 checks passed
@faucomte97 faucomte97 deleted the dev_container_clean_up branch March 6, 2024 14:17
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.

2 participants