EAGLE-1231: Added 'encoding' attribute to fields in Logical Graph schema

ICRAR / daliuge

The DALiuGE Execution Engine

GNU Lesser General Public License v2.1

24 stars 7 forks source link

EAGLE-1231: Added 'encoding' attribute to fields in Logical Graph schema #265

Closed james-strauss-uwa closed 5 days ago

james-strauss-uwa commented 3 months ago

Added a new encoding attribute to each field in the Logical Graph schema. Attribute is supported by EAGLE.

Allowed values are:

binary
pickle
dill
npy
base64
utf-8

Default value is "dill"

coveralls commented 3 months ago

coverage: 79.484% (+0.02%) from 79.469% when pulling 4ccb13db35223396169c27b06b1465db11ce662a on eagle-1231 into db91b75d352d9f90b45eebdda1de8a2d346d4a58 on master.

coveralls commented 3 months ago

coverage: 79.469%. remained the same when pulling aa73a5ac9ffda4c6a0378f3063bfda0b39b16086 on eagle-1231 into db91b75d352d9f90b45eebdda1de8a2d346d4a58 on master.

coveralls commented 1 week ago

coverage: 79.694%. remained the same when pulling 84216fe447a6945afbe7bb1fb6be2011f73699e1 on eagle-1231 into ea6cc063de04997ce713920c1023cd3cac8dca77 on master.

awicenec commented 1 week ago

Added a new encoding attribute to each field in the Logical Graph schema. Attribute is supported by EAGLE.

Allowed values are:

binary

pickle

dill

npy

base64

Default value is "dill"

Sorry, there obviously is a misunderstanding, what I meant is not to use utf-8 as the default, not to not have it at all. Could you please add it to the list again, since that is the only way to pass plain strings correctly.

james-strauss-uwa commented 1 week ago

@awicenec I've added 'utf-8' back to the options. And 'dill' is the default in the schema.

I'd be happy to merge this PR. I'm just a little concerned that github thinks it includes 4819 commits!

Would this be sorted out by the merge?

myxie commented 6 days ago

@james-strauss-uwa If you modify the merge branch from master to something else, and then back to master, that may resolve the erroneous number of commits.

james-strauss-uwa commented 6 days ago

@myxie Thanks for the suggestion, but it didn't seem to work.

Do you think we should merge anyway, or (since it is such as small change) create a new branch and PR?

myxie commented 6 days ago

@james-strauss-uwa on reflection the issue is due to the --force push that has been applied to the GitHub repo by @awicenec. This has likely over-ridden a significant number of commits that you have made and exist within the history of the commits within your branch. Ideally, we avoid --force push, as it leads to the complications discussed in https://www.atlassian.com/git/tutorials/merging-vs-rebasing:

This overwrites the remote main branch to match the rebased one from your repository and makes things very confusing for the rest of your team. .... Again, it’s important that nobody is working off of the commits from the original version of the feature branch.

The issue is that the force push has been applied whilst other branches were using the pre-existing commit IDs.

james-strauss-uwa commented 5 days ago

Created a new branch eagle-1231-2 and will create a new PR for that branch