-
Notifications
You must be signed in to change notification settings - Fork 2
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
[Missing Tokens] Write Directly to Spellbook File #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked like a charm! Thanks, Ben! New PR using the new code: duneanalytics/spellbook#3444
Left a minor test suggestion.
test_file = "./out/test_file.txt" | ||
try: | ||
os.mkdir("./out") | ||
open(test_file, "x") | ||
except FileExistsError: | ||
pass | ||
|
||
f = open(test_file, "a") | ||
file_lines = ["First Line\n", "Second Line\n"] | ||
f.write("".join(file_lines)) | ||
f.close() | ||
|
||
with open(test_file, "r", encoding="utf-8") as file: | ||
self.assertEqual(file_lines, file.readlines()) | ||
|
||
for i, old_line in enumerate(file_lines): | ||
new_line = f"New Line {i + 1}\n" | ||
replace_line(old_line, new_line, test_file) | ||
|
||
file_lines[i] = new_line | ||
with open(test_file, "r", encoding="utf-8") as file: | ||
self.assertEqual(file_lines, file.readlines()) | ||
os.remove(test_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, separating these three different use cases into separate methods for each case makes them easier to understand and maintain. What's your take on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya sure --- this isn't really a production product, but I could go and spend more time fixing this. Could be several days before I get back to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not gonna do that now. I need to start using this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: #34
just set the
SPELLBOOK_PATH
env var and away you go!Once we merge this it will be: