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

fix: change the encoding into utf-8 #8

Merged
merged 5 commits into from
Sep 15, 2023
Merged

Conversation

MistyField
Copy link
Contributor

No description provided.

@MistyField
Copy link
Contributor Author

MistyField commented Jul 29, 2023

Update: in the line 250 of parser.py refs is a string so when it was sliced, only 10 references in maximum could be added, and even worse, in the wrong order. So it may be changed into references[refs] = line and it was tested fine.

@Robaina
Copy link
Owner

Robaina commented Sep 10, 2023

Thanks so much for the PR!

I'm planning to update BRENDApyrser to accommodate the newly available json format, see issue #9. I'll leave this PR on hold for now since most likely this issue with the references will be solved.

@MistyField
Copy link
Contributor Author

MistyField commented Sep 10, 2023

Thanks so much for the PR!

I'm planning to update BRENDApyrser to accommodate the newly available json format, see issue #9. I'll leave this PR on hold for now since most likely this issue with the references will be solved.

Thank you for your reply! And I successfully made a database of enzyme kinetics using BRENDApyrser which is very powerful! For me, parsing the json is a good idea, it will save a lot of time in parsing the data, but the change in this commit is needed regardless of whether the data format is .txt or .json since it will impact the consistency of the content in the references.

Best regards,

@Robaina Robaina self-requested a review September 11, 2023 15:23
@Robaina
Copy link
Owner

Robaina commented Sep 11, 2023

Glad to hear that! It's awesome to learn that you have found the package useful!

@Robaina
Copy link
Owner

Robaina commented Sep 13, 2023

Hi @MistyField ,

sorry, could you update your branch with the current master branch to resolve these conflicts prior to merge?

@MistyField MistyField closed this Sep 14, 2023
@MistyField MistyField reopened this Sep 14, 2023
@MistyField
Copy link
Contributor Author

Hi @MistyField ,

sorry, could you update your branch with the current master branch to resolve these conflicts prior to merge?

Hello, I've updated my forked branch with the master branch:)

@Robaina Robaina added the enhancement New feature or request label Sep 15, 2023
Copy link
Owner

@Robaina Robaina left a comment

Choose a reason for hiding this comment

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

I don't want to merge this commit with the ".idea" directory from JetBrains? This is a local directory, shouldn't be in the repo. Please, could you remove it? We don't need to include ".idea" in .gitignore either.

Otherwise, I'm fine with the rest of the PR!

@MistyField
Copy link
Contributor Author

I don't want to merge this commit with the ".idea" directory from JetBrains? This is a local directory, shouldn't be in the repo. Please, could you remove it? We don't need to include ".idea" in .gitignore either.

Otherwise, I'm fine with the rest of the PR!

Of course! Apologize for the inconvenience I caused by changing the .gitignore🙏

Copy link
Owner

@Robaina Robaina left a comment

Choose a reason for hiding this comment

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

Excellent! Thank you!

@Robaina
Copy link
Owner

Robaina commented Sep 15, 2023

I don't want to merge this commit with the ".idea" directory from JetBrains? This is a local directory, shouldn't be in the repo. Please, could you remove it? We don't need to include ".idea" in .gitignore either.
Otherwise, I'm fine with the rest of the PR!

Of course! Apologize for the inconvenience I caused by changing the .gitignore🙏

No worries ! Merging PR

@Robaina Robaina merged commit 7137aa0 into Robaina:master Sep 15, 2023
1 check passed
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