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

Declare exercises in toml files instead of d files #10

Merged
merged 5 commits into from
Feb 14, 2023

Conversation

iK4tsu
Copy link
Owner

@iK4tsu iK4tsu commented Feb 1, 2023

No description provided.

@iK4tsu iK4tsu added the enhancement New feature or request label Feb 1, 2023
@iK4tsu
Copy link
Owner Author

iK4tsu commented Feb 1, 2023

Fixes #7

@iK4tsu iK4tsu force-pushed the refactor-define-exercises-in-toml branch 2 times, most recently from b716266 to ace457f Compare February 1, 2023 21:21
source/app.d Outdated Show resolved Hide resolved
source/app.d Show resolved Hide resolved
source/info.d Show resolved Hide resolved
source/info.d Show resolved Hide resolved
source/app.d Outdated
return;
}

immutable Exercise[] exercises = "exercises.toml".exercisesFromTOML();
Copy link

Choose a reason for hiding this comment

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

Since you are parsing UTF-8 text, you can handle a custom message like you done above for .toml files that are not UTF-8 compliant.

Copy link
Owner Author

Choose a reason for hiding this comment

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

could you further explain?

Copy link

Choose a reason for hiding this comment

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

readText throws an exception on bad UTF-8 reading. You can handle it to pretty print an error message rather than the default exception message + stacktrace, like you have done above.

Copy link
Owner Author

Choose a reason for hiding this comment

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

where is that information stated?

Copy link

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Does the current change look alright?

source/info.d Outdated Show resolved Hide resolved
source/info.d Outdated Show resolved Hide resolved
@ljmf00
Copy link

ljmf00 commented Feb 2, 2023

Wouldn't be interesting to parse TOML and use import() expression, so you can have them accessible at compile-time too, or do you want to make this so you can dynamically add exercises?

@iK4tsu
Copy link
Owner Author

iK4tsu commented Feb 2, 2023

Wouldn't be interesting to parse TOML and use import() expression, so you can have them accessible at compile-time too, or do you want to make this so you can dynamically add exercises?

No. That invalidates the whole purpose of this change. What would be the difference between having the exercises defined as they were in info.d and importing the file at compile time? The goal is to not force users to recompile the project each time modifications are done to exercises. Dlings should not have to be recompiled in case of a newly added exercise, a removed exercise, or if any "metadata" is modified (hint, name, path, ...)

@ljmf00
Copy link

ljmf00 commented Feb 2, 2023

Wouldn't be interesting to parse TOML and use import() expression, so you can have them accessible at compile-time too, or do you want to make this so you can dynamically add exercises?

No. That invalidates the whole purpose of this change. What would be the difference between having the exercises defined as they were in info.d and importing the file at compile time? The goal is to not force users to recompile the project each time modifications are done to exercises. Dlings should not have to be recompiled in case of a newly added exercise, a removed exercise, or if any "metadata" is modified (hint, name, path, ...)

Not importing the exercises itself, but rather just including the metadata. But it makes total sense nevertheless

@iK4tsu iK4tsu force-pushed the refactor-define-exercises-in-toml branch from 0bde995 to 9fc0fb8 Compare February 4, 2023 21:41
@iK4tsu iK4tsu force-pushed the refactor-define-exercises-in-toml branch 2 times, most recently from 5d9cc81 to 0e68c4d Compare February 5, 2023 23:29
@iK4tsu
Copy link
Owner Author

iK4tsu commented Feb 5, 2023

update:

  • added a simple Result implementation in util.d
  • handled exceptions for better error messages, instead of showing an ugly stack trace

@iK4tsu iK4tsu force-pushed the refactor-define-exercises-in-toml branch from 0e68c4d to b20071b Compare February 5, 2023 23:39
@iK4tsu
Copy link
Owner Author

iK4tsu commented Feb 5, 2023

update:

  • added attributes to Result
  • added more Result unit tests

@iK4tsu iK4tsu force-pushed the refactor-define-exercises-in-toml branch from b20071b to 9af7467 Compare February 6, 2023 09:43
"argparse": "~master",
"fswatch": "0.6.0",
"toml": "2.0.1",
"unit-threaded": "2.0.5"
Copy link

Choose a reason for hiding this comment

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

CC @WebFreak001 this shouldn't be there. I can make a PR if I have time.

Copy link

Choose a reason for hiding this comment

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

Humm, the dub config on toml seems correct 🤔 . Maybe its a behavior of dub, but I suppose this dependency doesn't affect the outer dependency, so no need to be version pinned, because it's only used internally. Perhaps have a dedicated internal-unittest configuration on dub config?

source/util.d Outdated Show resolved Hide resolved
source/util.d Outdated Show resolved Hide resolved
source/util.d Outdated Show resolved Hide resolved
source/util.d Show resolved Hide resolved
@iK4tsu iK4tsu force-pushed the refactor-define-exercises-in-toml branch from 9af7467 to 8a6a25f Compare February 14, 2023 19:56
@iK4tsu
Copy link
Owner Author

iK4tsu commented Feb 14, 2023

update:

  • fixed typo
  • added a get Result method

@iK4tsu iK4tsu merged commit 7122760 into master Feb 14, 2023
@iK4tsu iK4tsu deleted the refactor-define-exercises-in-toml branch February 14, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants